Skip to content

Web: Make s3 fields optional#40145

Merged
kimlisa merged 9 commits intomasterfrom
lisa/web-s3-bucket-warn
Apr 9, 2024
Merged

Web: Make s3 fields optional#40145
kimlisa merged 9 commits intomasterfrom
lisa/web-s3-bucket-warn

Conversation

@kimlisa
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa commented Apr 2, 2024

requires

Description:

  • When creating AND editing integration: allow s3 fields to be optional, if user leaves it empty, render a warning
  • When listing integrations without s3 fields defined, renders a tooltip that provides information to the user on how to update their thumbprint should they run into issues

listing:
image

listing, after clicking on button:
image

editing:
image

creating:
image

changelog: Amazon S3 fields are now optional when creating or editing AWS OIDC integration on the web UI

@github-actions github-actions Bot requested review from avatus and rudream April 2, 2024 23:02
@kimlisa kimlisa added no-changelog Indicates that a PR does not require a changelog entry and removed no-changelog Indicates that a PR does not require a changelog entry labels Apr 2, 2024
@gravitational gravitational deleted a comment from github-actions Bot Apr 2, 2024
@gravitational gravitational deleted a comment from github-actions Bot Apr 2, 2024
@gravitational gravitational deleted a comment from github-actions Bot Apr 2, 2024
Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get to run it e2e yet but code looks good

Comment thread web/packages/design/src/Alert/Alert.jsx Outdated
background: fade(theme.colors.link, 0.1),
border: `${theme.radii[1]}px solid ${theme.colors.link}`,
background: fade(theme.colors.accent.main, 0.1),
border: `${theme.radii[1]}px solid ${theme.colors.accent.main}`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:
using radii as a border value could bite us in the future if we decided "lets make things more round" and then all of a sudden this border is thicker too. I think the value 2px here would be better

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the use case for radii then?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be specific to border-radius. Also, I didn't see this before, but our theme has theme.borders so we could just do

Suggested change
border: `${theme.radii[1]}px solid ${theme.colors.accent.main}`,
border: `${theme.borders[2]} ${theme.colors.accent.main}`,

check the sharedStyles here

@kimlisa
Copy link
Copy Markdown
Contributor Author

kimlisa commented Apr 4, 2024

friendly ping @rudream (i'll apply michaels change right before i merge)

@kimlisa kimlisa force-pushed the lisa/web-s3-bucket-warn branch from be460e0 to 77f47a9 Compare April 8, 2024 20:08
@kimlisa kimlisa enabled auto-merge April 9, 2024 17:03
@kimlisa kimlisa added this pull request to the merge queue Apr 9, 2024
Merged via the queue into master with commit e3a8e17 Apr 9, 2024
@kimlisa kimlisa deleted the lisa/web-s3-bucket-warn branch April 9, 2024 17:21
@public-teleport-github-review-bot
Copy link
Copy Markdown

@kimlisa See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Create PR

@codingllama
Copy link
Copy Markdown
Contributor

FYI, this broke make lint-license, which breaks Go PRs like #40337. It's likely that the linter isn't always running when it should.

github-merge-queue Bot pushed a commit that referenced this pull request Apr 9, 2024
* Web: Make s3 fields optional (#40145)

* Fix import

* Fix license lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants