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.

Sign up for GitHub

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

Jump to bottom

feat(artifacts): Add type parameter to ArtifactService #821

Merged
merged 2 commits into from
Jul 22, 2020
Merged

feat(artifacts): Add type parameter to ArtifactService #821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
806d66d
feat(artifacts): Add type parameter to ArtifactService
luispollo Jul 21, 2020
22692c5
fix(pr): Address review feedback + allow querying multiple statuses
luispollo Jul 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions igor-web/src/main/java/com/netflix/spinnaker/igor/artifacts/ArtifactController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

love it, so great!

@RequestParam(value = "type", required = false, defaultValue = "deb") String type) {
Copy link
Contributor Author

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.

ArtifactService artifactService = getService(provider);
return artifactService.getArtifactVersions(name, releaseStatus);
return artifactService.getArtifactVersions(type, name, releaseStatuses);
}

@GetMapping("/{provider}/{name}/{version:.+}")
public Artifact getArtifact(
@PathVariable("provider") String provider,
@PathVariable("name") String name,
@PathVariable("version") String version) {
@PathVariable("version") String version,
@RequestParam(value = "type", required = false, defaultValue = "deb") String type) {
ArtifactService artifactService = getService(provider);
return artifactService.getArtifact(name, version);
return artifactService.getArtifact(type, name, version);
}

private ArtifactService getService(String serviceName) {
Expand Down
15 changes: 13 additions & 2 deletions igor-web/src/main/java/com/netflix/spinnaker/igor/artifacts/ArtifactService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import com.netflix.spinnaker.igor.model.ArtifactServiceProvider;
import com.netflix.spinnaker.kork.artifacts.model.Artifact;
import java.util.ArrayList;
import java.util.List;
import javax.annotation.Nonnull;

Expand All @@ -33,9 +34,19 @@ public interface ArtifactService {

/** Used to populate the manual trigger dropdown with options */
@Nonnull
List<String> getArtifactVersions(@Nonnull String name, String releaseStatus);
default List<String> getArtifactVersions(
@Nonnull String type, @Nonnull String name, String releaseStatus) {
List<String> releaseStatuses = new ArrayList<>();
releaseStatuses.add(releaseStatus);
return getArtifactVersions(type, name, releaseStatuses);
}

/** Used to populate the manual trigger dropdown with options */
@Nonnull
List<String> getArtifactVersions(
@Nonnull String type, @Nonnull String name, List<String> releaseStatuses);
Copy link
Contributor Author

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.


/** Used to fetch a specific artifact for decorating a trigger */
@Nonnull
Artifact getArtifact(@Nonnull String name, @Nonnull String version);
Artifact getArtifact(@Nonnull String type, @Nonnull String name, @Nonnull String version);
luispollo marked this conversation as resolved.
Show resolved Hide resolved
}
16 changes: 8 additions & 8 deletions igor-web/src/test/groovy/com/netflix/spinnaker/igor/artifacts/ArtifactServiceSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ class ArtifactServiceSpec extends Specification {

@Subject
ArtifactServices artifactServices = new ArtifactServices()

void setup() {
Map<String, ArtifactService> services = new HashMap<>()
services.put("artifactory", new TestArtifactService())
artifactServices.addServices(services)
}

def "finds matching service"() {
when:
def service = artifactServices.getService("artifactory")
Expand All @@ -53,7 +53,7 @@ class ArtifactServiceSpec extends Specification {
def "service finds artifact versions"() {
when:
def service = artifactServices.getService("artifactory")
def versions = service.getArtifactVersions("test", null)
def versions = service.getArtifactVersions("deb","test", null)

then:
assertThat(versions).isNotNull()
Expand All @@ -64,7 +64,7 @@ class ArtifactServiceSpec extends Specification {
def "service finds only snapshot artifacts"() {
when:
def service = artifactServices.getService("artifactory")
def versions = service.getArtifactVersions("test", "snapshot")
def versions = service.getArtifactVersions("deb","test", "snapshot")

then:
assertThat(versions).isNotNull()
Expand All @@ -75,7 +75,7 @@ class ArtifactServiceSpec extends Specification {
def "service finds artifact"() {
when:
def service = artifactServices.getService("artifactory")
def artifact = service.getArtifact("test", "v0.4.0")
def artifact = service.getArtifact("deb","test", "v0.4.0")

then:
assertThat(artifact).isNotNull()
Expand All @@ -86,7 +86,7 @@ class ArtifactServiceSpec extends Specification {
def "versions list is empty when no versions found"() {
when:
def service = artifactServices.getService("artifactory")
def versions = service.getArtifactVersions("blah", "")
def versions = service.getArtifactVersions("deb","blah", "")

then:
assertThat(versions).isNotNull()
Expand All @@ -96,10 +96,10 @@ class ArtifactServiceSpec extends Specification {
def "404 is thrown when artifact not found"() {
when:
def service = artifactServices.getService("artifactory")
service.getArtifact("blah","v0.0.1")
service.getArtifact("deb","blah","v0.0.1")

then:
thrown(NotFoundException)
}

}
10 changes: 6 additions & 4 deletions igor-web/src/test/java/com/netflix/spinnaker/igor/artifacts/TestArtifactService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,25 +31,27 @@ public ArtifactServiceProvider artifactServiceProvider() {
}

@Override
public List<String> getArtifactVersions(String name, String releaseStatus) {
public List<String> getArtifactVersions(String type, String name, List<String> releaseStatuses) {
if (!name.equals("test")) {
return Collections.emptyList();
}
List<String> versions = new ArrayList<>();
if (releaseStatus == null || releaseStatus.isEmpty() || releaseStatus.contains("final")) {
if (releaseStatuses == null || releaseStatuses.isEmpty() || releaseStatuses.contains("final")) {
versions.add("v0.1.0");
versions.add("v0.2.0");
versions.add("v0.3.0");
versions.add("v0.4.0");
}
if (releaseStatus == null || releaseStatus.isEmpty() || releaseStatus.contains("snapshot")) {
if (releaseStatuses == null
|| releaseStatuses.isEmpty()
|| releaseStatuses.contains("snapshot")) {
versions.add("v0.5.0~SNAPSHOT");
}
return versions;
}

@Override
public Artifact getArtifact(String name, String version) {
public Artifact getArtifact(String type, String name, String version) {
if (!name.equals("test") && !version.equals("v0.4.0")) {
throw new NotFoundException("Artifact not found");
}
Expand Down