-
Notifications
You must be signed in to change notification settings - Fork 658
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(artifacts): Add type parameter to ArtifactService #821
Conversation
44a8e4b
to
af347c1
Compare
@@ -46,18 +46,20 @@ public ArtifactController(ArtifactServices artifactServices) { | |||
public List<String> getVersions( | |||
@PathVariable("provider") String provider, | |||
@PathVariable("name") String name, | |||
@RequestParam(value = "releaseStatus", required = false) String releaseStatus) { | |||
@RequestParam(value = "releaseStatus", required = false) String releaseStatus, | |||
@RequestParam(value = "type", required = false, defaultValue = "deb") String type) { |
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.
I added this as a request param and not a path variable because it would cause a conflict with between the path patterns in getArtifact
and getVersions
.
igor-web/src/main/java/com/netflix/spinnaker/igor/artifacts/ArtifactService.java
Show resolved
Hide resolved
igor-web/src/main/java/com/netflix/spinnaker/igor/artifacts/ArtifactService.java
Outdated
Show resolved
Hide resolved
@@ -46,18 +46,20 @@ public ArtifactController(ArtifactServices artifactServices) { | |||
public List<String> getVersions( | |||
@PathVariable("provider") String provider, | |||
@PathVariable("name") String name, | |||
@RequestParam(value = "releaseStatus", required = false) String releaseStatus) { | |||
@RequestParam(value = "releaseStatus", required = false) List<String> releaseStatuses, |
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.
This was previously limiting the ability to query artifacts matching multiple statuses.
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.
Is this change backwards compatible with the manual trigger dropdown only sending one status?
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.
Yes, because the way this is encoded in query parameters when it's a list is to have multiple occurrences of releaseStatus=<status>
in the query string. This works for 1 to N.
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.
love it, so great!
@@ -33,9 +33,10 @@ | |||
|
|||
/** Used to populate the manual trigger dropdown with options */ | |||
@Nonnull | |||
List<String> getArtifactVersions(@Nonnull String name, String releaseStatus); | |||
List<String> getArtifactVersions( | |||
@Nonnull String type, @Nonnull String name, List<String> releaseStatuses); |
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.
This was previously limiting the ability to query artifacts with multiple statuses.
aaa4e62
to
f583ce0
Compare
f583ce0
to
22692c5
Compare
Adds a
type
parameter to theArtifactService
interfaces methods such that we can query different types of artifacts (not just Debian).