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

feat(repository): disallow duplicate artifact refs #841

Merged
merged 5 commits into from
Mar 4, 2020

Conversation

lorin
Copy link
Contributor

@lorin lorin commented Feb 28, 2020

Enforce that all artifacts in a delivery configuration have a unique reference.

Fixes #761.

If a user attempts to submit a delivery configuration that violates this, the response payload looks like this:

---
message: "Multiple artifacts are using the same reference: {lhochstein/lorintesttitus=my-artifact,\
  \ emburns/spin-titus-demo=my-artifact}. Please ensure each artifact has a unique\
  \ reference."

(As an aside, I don't think keel should return a 500 error on input that fails validation, but that's the current behavior). (My mistake: returning 500 was not the previous behavior on a validation error)

It will also log a similar error message at the ERROR level:

Multiple artifacts are using the same reference: {lhochstein/lorintesttitus=my-artifact, emburns/spin-titus-demo=my-artifact}. Please ensure each artifact has a unique reference.

@lorin
Copy link
Contributor Author

lorin commented Feb 28, 2020

@luispollo @asher

@lorin
Copy link
Contributor Author

lorin commented Mar 2, 2020

Rebased against master

@luispollo
Copy link
Contributor

(As an aside, I don't think keel should return a 500 error on input that fails validation, but that's the current behavior).

I agree. In general we've bee using 400 in that case. Take a look at the ExceptionHandler class -- it converts known exception types into the desired status code/response. You might need to add another exception type to the 400 case.

Lorin Hochstein and others added 5 commits March 2, 2020 15:56
Enforce that all artifacts in a delivery configuration have a unique
reference.

Fixes spinnaker#761
Use named function instead of lambda for clarity.

Use associate instead of map + toMap
Return a 400 on any validation error
Co-Authored-By: Luis Fernando Pollo <1323478+luispollo@users.noreply.github.com>
@lorin
Copy link
Contributor Author

lorin commented Mar 2, 2020

rebase against master

Copy link
Contributor

@luispollo luispollo left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this and congrats on your first Spinnaker PR! 🎉

@luispollo luispollo added the ready to merge Approved and ready for merge label Mar 4, 2020
@mergify mergify bot merged commit 92907d3 into spinnaker:master Mar 4, 2020
@mergify mergify bot added the auto merged label Mar 4, 2020
@luispollo
Copy link
Contributor

P.S. It would be nice to add a test case to ExceptionHandlerTests to cover the new handling of ValidationException in another PR. Sorry I didn't catch this earlier.

@lorin lorin deleted the dup-art-refs branch March 4, 2020 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged ready to merge Approved and ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure that artifact reference is unique across submitted delivery configs
3 participants