Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: Remove KV pre-flight auth check #11968

Merged
merged 3 commits into from Jan 7, 2022
Merged

Conversation

johncowen
Copy link
Contributor

Related to #11098

This PR removes the KV pre-flight check to the internal authorization endpoint and instead relies on the response from the normal KV endpoint, thin client style.

@johncowen johncowen added theme/ui Anything related to the UI backport/1.10 labels Jan 7, 2022
@johncowen johncowen requested review from jgwhite, amyrlam, a user and natmegs January 7, 2022 17:27
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Not sure if you have a feature flag system in place but could this be a feature flag? That may make it more visible so it's not forgotten. Also a small change so might not be worth it.

@johncowen
Copy link
Contributor Author

👍 ty!

Yeah we could have feature flagged this with code I suppose. I kinda have here, I'm planning on just search for the issue number once the backend issue is resolved, which is kind of like a poor mans feature flag, hence the extra comments in the code I've not changed. tbh I'm not even sure when the backend API issue is going to be resolved.

I suppose also code based feature flagging would involve injecting our env service, adding the flags, probably deciding on what to call the flag etc etc, basically a bunch of code that we will just remove once the backend it fixed. I guess I'm thinking code based feature flagging would be more for being able to turn things on and off and on and off etc etc multiple times, or maybe to be able to work on a feature only in certain environments, not just for enabling something once its working and thats the end of it.

@vercel vercel bot temporarily deployed to Preview – consul January 7, 2022 19:00 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging January 7, 2022 19:00 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging January 7, 2022 19:19 Inactive
@vercel vercel bot temporarily deployed to Preview – consul January 7, 2022 19:19 Inactive
@johncowen johncowen merged commit d9a315e into main Jan 7, 2022
@johncowen johncowen deleted the ui/bugfix/kv-listing-auth branch January 7, 2022 19:26
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/543441.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit d9a315e onto release/1.11.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Jan 7, 2022
* ui: Don't even ask whether we are authorized for a KV...

...just let the actual API tell us in the response, thin-client style.

* Add some similar commenting for previous PRs related to this problem
@hc-github-team-consul-core
Copy link
Collaborator

🍒❌ Cherry pick of commit d9a315e onto release/1.10.x failed! Build Log

johncowen added a commit that referenced this pull request Jan 7, 2022
* ui: Don't even ask whether we are authorized for a KV...

...just let the actual API tell us in the response, thin-client style.

* Add some similar commenting for previous PRs related to this problem
johncowen added a commit that referenced this pull request Jan 10, 2022
* ui: Don't even ask whether we are authorized for a KV...

...just let the actual API tell us in the response, thin-client style.

* Add some similar commenting for previous PRs related to this problem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants