Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add support to validate via Kong API #502
feat: add support to validate via Kong API #502
Changes from 1 commit
231e0e7
b91c844
ef450c4
4136209
422f066
bc31676
e25214e
a4c603e
9c3ec69
94ed516
ddeb657
7523bfd
a6b0a11
34fb225
3e13393
6319328
536ba04
809e7d3
9c68a3b
69135ea
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get rid off this function and call validateEntity from validateEntities directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My one last product review comment: it must be possible for the user to validate their configuration without the existence of a workspace. It is usual for our users to validate their configuration before applying. This is a blocker for merging.
cc @rainest can help figure out alternative solutions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some workspace has to exist for you to check :)
You could have this fall back to trying against default if the workspace requested isn't available. It may fail, as you may not have permissions--using
--workspace
here implies you don't, since otherwise you could omit the flag and check against default fine.Users may try and just always supply
--workspace
if that's their intended target for lack of understanding that it's only needed here to work around RBAC issues, since that's a fairly nuanced point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rainest Can we open a Github issue with the relevant context to track this problem? We must improve this later if not in this PR.
The PR can be merged after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So
deck
already assumes the default workspace always exists: https://github.com/Kong/deck/blob/main/cmd/common.go#L31-L35What we can do is simply overwriting the user input from
--workspace
and set it to the default while printing a warning:If that is fine to you, I can push the changes, so that we don't have to open a different issue and tackle it at a different time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GGabriele Please open an issue to track this problem. Can skip for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#563