Skip to content

add validation to argocd app set -p#309

Merged
JazminElkan-Gonzalez merged 2 commits intoargoproj:masterfrom
JazminElkan-Gonzalez:issue195
Jun 22, 2018
Merged

add validation to argocd app set -p#309
JazminElkan-Gonzalez merged 2 commits intoargoproj:masterfrom
JazminElkan-Gonzalez:issue195

Conversation

@JazminElkan-Gonzalez
Copy link
Contributor

This fixes #195

I rebased to clean up commit history but change was contributed by @dthomson25

Util function checks if a given parameter matches component and name to a parameter in the given application.

server side "updateSpec" function uses util to verify passed override parameters along with all previous override parameters.

client side "app set" function also uses util to verify passed override parameter before calling "updateSpec"

throws error if passed parameter is illegal, warns users if any old overrides are dropped

Copy link
Member

@dthomson25 dthomson25 Jun 21, 2018

Choose a reason for hiding this comment

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

Do we want to extract this into the Utils since it's the same code on server and cli?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we put this into CheckValidParam than we have to loop through it for every parameter, I put it here so we would only have to loop through it once per UpdateSpec. rather than once per parameter.

I could make a Util that is "convertToMap" that does the loop as a separate function.

Copy link
Member

Choose a reason for hiding this comment

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

  1. The error check that used to come immediately after the Applications().Get() request, now comes after this large code block. So its possible to get a nil pointer exception while you are attempting to validate the params (a might be nil).

  2. I think this validation logic is large enough that it should be put into a separate function to keep the UpdateSpec more concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. ah good point will reorder.
  2. ok, makes sense will create a helper function tomorrow.

thanks for the feedback!

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the need to do this client side validation if we are already doing this server side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm the thought was to have it fail fast (before it even gets to the server side logic). but given that it's a relatively costly process / to keep things clean, I think it makes sense to omit this.

@JazminElkan-Gonzalez
Copy link
Contributor Author

@jessesuen I made the changes we spoke about, here is some example outputs.

image

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

LGTM! please squash commits when merging

@JazminElkan-Gonzalez JazminElkan-Gonzalez merged commit 4353f73 into argoproj:master Jun 22, 2018
@JazminElkan-Gonzalez JazminElkan-Gonzalez deleted the issue195 branch June 25, 2018 21:11
leoluz pushed a commit to leoluz/argo-cd that referenced this pull request Sep 29, 2023
…tion (argoproj#309)

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

argocd app set -p to have validation

3 participants