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

fix(pipelineTriggers): change buildNumber type from int to string #1049

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(pipelineTriggers): change buildNumber type from int to string #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d605c1f
fix(pipelineTriggers): change buildNumber type from int to string
Oct 20, 2022
72a1180
fix tests
Oct 20, 2022
00d4641
clearer method for converting to string
Nov 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions igor-core/src/main/java/com/netflix/spinnaker/igor/build/BuildCache.java
View file
Open in desktop
Expand Up @@ -71,14 +71,14 @@ public List<String> getTypeaheadResults(String search) {
return results;
}

public int getLastBuild(String master, String job, boolean running) {
public String getLastBuild(String master, String job, boolean running) {
String key = makeKey(master, job, running);
return redisClientDelegate.withCommandsClient(
c -> {
if (!c.exists(key)) {
return -1;
return "-1";
}
return Integer.parseInt(c.get(key));
return c.get(key);
});
}

Expand All @@ -101,7 +101,7 @@ public void setTTL(String key, int ttlSeconds) {
});
}

public void setLastBuild(String master, String job, int lastBuild, boolean building, int ttl) {
public void setLastBuild(String master, String job, String lastBuild, boolean building, int ttl) {
if (!building) {
setBuild(makeKey(master, job), lastBuild, false, master, job, ttl);
}
Expand Down Expand Up @@ -182,19 +182,19 @@ private static Map<String, String> getTrackedBuild(String key) {
}

private void setBuild(
String key, int lastBuild, boolean building, String master, String job, int ttl) {
String key, String lastBuild, boolean building, String master, String job, int ttl) {
redisClientDelegate.withCommandsClient(
c -> {
c.hset(key, "lastBuildLabel", Integer.toString(lastBuild));
c.hset(key, "lastBuildLabel", lastBuild);
c.hset(key, "lastBuildBuilding", Boolean.toString(building));
});
setTTL(key, ttl);
}

private void storeLastBuild(String key, int lastBuild, int ttl) {
private void storeLastBuild(String key, String lastBuild, int ttl) {
redisClientDelegate.withCommandsClient(
c -> {
c.set(key, Integer.toString(lastBuild));
c.set(key, lastBuild);
});
setTTL(key, ttl);
}
Expand Down
2 changes: 1 addition & 1 deletion igor-core/src/main/java/com/netflix/spinnaker/igor/build/model/GenericBuild.java
View file
Open in desktop
Expand Up @@ -35,7 +35,7 @@ public class GenericBuild {
private boolean building;
private String fullDisplayName;
private String name;
private int number;
private String number;
private Integer duration;
/** String representation of time in nanoseconds since Unix epoch */
private String timestamp;
Expand Down
4 changes: 2 additions & 2 deletions igor-core/src/main/java/com/netflix/spinnaker/igor/service/BuildOperations.java
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ public interface BuildOperations extends BuildService {
* @param buildNumber The build number
* @return A Spinnaker representation of a build
*/
GenericBuild getGenericBuild(String job, int buildNumber);
GenericBuild getGenericBuild(String job, String buildNumber);

/**
* Trigger a build of a given job on the build service host
Expand All @@ -71,7 +71,7 @@ public interface BuildOperations extends BuildService {
* @param buildNumber The build number
* @param updatedBuild The updated details for the build
*/
default void updateBuild(String jobName, Integer buildNumber, UpdatedBuild updatedBuild) {
default void updateBuild(String jobName, String buildNumber, UpdatedBuild updatedBuild) {
// not supported by default
}

Expand Down
19 changes: 11 additions & 8 deletions igor-monitor-travis/src/main/java/com/netflix/spinnaker/igor/travis/TravisBuildMonitor.java
View file
Open in desktop
Expand Up @@ -178,11 +178,11 @@ protected void commitDelta(BuildPollingDelta delta, boolean sendEvents) {

private Stream<? extends BuildDelta> createBuildDelta(
String master, TravisService travisService, V3Build v3Build) {
int lastBuild =
String lastBuild =
buildCache.getLastBuild(master, v3Build.branchedRepoSlug(), v3Build.getState().isRunning());
return Stream.of(v3Build)
.filter(build -> !build.spinnakerTriggered())
.filter(build -> build.getNumber() > lastBuild)
.filter(build -> build.getNumber().compareTo(lastBuild) > 0)
.map(
build ->
BuildDelta.builder()
Expand Down Expand Up @@ -212,9 +212,12 @@ private void processBuild(
if (!travisService.isLogReady(build)) {
break;
}
if (build.getNumber()
> buildCache.getLastBuild(
master, build.getRepository().getSlug(), build.getState().isRunning())) {
if (build
.getNumber()
.compareTo(
Copy link
Member

Choose a reason for hiding this comment

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

Just a minor concern... since this is looking for making sure there aren't new builds to trigger, the compareTo with this being string vs. int now may not behave exactly the way intended. I've not checked everyplace on this PR yet, was reading through/glancing through and type conversions where this happens... since it's a string comparison we're going to see very different results...

buildCache.getLastBuild(
master, build.getRepository().getSlug(), build.getState().isRunning()))
> 0) {
buildCache.setLastBuild(
master,
build.getRepository().getSlug(),
Expand All @@ -226,7 +229,7 @@ private void processBuild(
}
}

if (build.getNumber() > item.previousBuildNum) {
if (build.getNumber().compareTo(item.previousBuildNum) > 0) {
buildCache.setLastBuild(
master,
build.branchedRepoSlug(),
Expand Down Expand Up @@ -311,7 +314,7 @@ public static class BuildDelta implements DeltaItem {
private final V3Build build;
private final GenericBuild genericBuild;
private final String travisBaseUrl;
private final int currentBuildNum;
private final int previousBuildNum;
private final String currentBuildNum;
private final String previousBuildNum;
}
}
2 changes: 1 addition & 1 deletion igor-monitor-travis/src/main/java/com/netflix/spinnaker/igor/travis/client/TravisClient.java
View file
Open in desktop
Expand Up @@ -57,7 +57,7 @@ public interface TravisClient {
public abstract Builds builds(
@Header("Authorization") String accessToken,
@Query("slug") String repoSlug,
@Query("number") int buildNumber);
@Query("number") String buildNumber);

@POST("/repo/{repoSlug}/requests")
@Headers("Travis-API-Version: 3")
Expand Down
2 changes: 1 addition & 1 deletion igor-monitor-travis/src/main/java/com/netflix/spinnaker/igor/travis/client/model/Build.java
View file
Open in desktop
Expand Up @@ -43,7 +43,7 @@ public class Build {
@JsonProperty("repository_id")
private int repositoryId;

private int number;
private String number;

private TravisBuildState state;

Expand Down
2 changes: 1 addition & 1 deletion ...nitor-travis/src/main/java/com/netflix/spinnaker/igor/travis/client/model/v3/V3Build.java
View file
Open in desktop
Expand Up @@ -57,7 +57,7 @@ public class V3Build {
@JsonProperty("repository_id")
private int repositoryId;

private int number;
private String number;

@EqualsAndHashCode.Include private TravisBuildState state;

Expand Down
10 changes: 5 additions & 5 deletions ...monitor-travis/src/main/java/com/netflix/spinnaker/igor/travis/service/TravisService.java
View file
Open in desktop
Expand Up @@ -168,7 +168,7 @@ public List<GenericGitRevision> getGenericGitRevisions(String inputRepoSlug, Gen
}

@Override
public GenericBuild getGenericBuild(final String inputRepoSlug, final int buildNumber) {
public GenericBuild getGenericBuild(final String inputRepoSlug, final String buildNumber) {
String repoSlug = cleanRepoSlug(inputRepoSlug);
Build build = getBuild(repoSlug, buildNumber);
return getGenericBuild(build, repoSlug);
Expand Down Expand Up @@ -215,11 +215,11 @@ public V3Build getV3Build(int buildId) {
getAccessToken(), buildId, addLogCompleteIfApplicable("build.commit"));
}

public Builds getBuilds(String repoSlug, int buildNumber) {
public Builds getBuilds(String repoSlug, String buildNumber) {
return travisClient.builds(getAccessToken(), repoSlug, buildNumber);
}

public Build getBuild(String repoSlug, int buildNumber) {
public Build getBuild(String repoSlug, String buildNumber) {
Builds builds = getBuilds(repoSlug, buildNumber);
return !builds.getBuilds().isEmpty() ? builds.getBuilds().get(0) : null;
}
Expand Down Expand Up @@ -505,7 +505,7 @@ public String getUrl(String repoSlug) {
}

@Override
public Map<String, Integer> queuedBuild(String master, int queueId) {
public Map<String, String> queuedBuild(String master, int queueId) {
Map<String, Integer> queuedJob = travisCache.getQueuedJob(groupKey, queueId);
Request requestResponse =
travisClient.request(
Expand All @@ -519,7 +519,7 @@ public Map<String, Integer> queuedBuild(String master, int queueId) {
queueId,
groupKey);
travisCache.removeQuededJob(groupKey, queueId);
LinkedHashMap<String, Integer> map = new LinkedHashMap<>(1);
LinkedHashMap<String, String> map = new LinkedHashMap<>(1);
map.put("number", requestResponse.getBuilds().get(0).getNumber());
return map;
}
Expand Down
76 changes: 40 additions & 36 deletions ...or-travis/src/test/groovy/com/netflix/spinnaker/igor/travis/TravisBuildMonitorSpec.groovy
View file
Open in desktop
Expand Up @@ -76,21 +76,21 @@ class TravisBuildMonitorSpec extends Specification {
1 * travisService.getLatestBuilds() >> [ build ]
build.branchedRepoSlug() >> "test-org/test-repo/master"
build.jobs >> []
build.getNumber() >> 4
build.getNumber() >> '4'
build.getState() >> TravisBuildState.passed
build.repository >> repository
repository.slug >> 'test-org/test-repo'

1 * travisService.getGenericBuild(build, true) >> TravisBuildConverter.genericBuild(build, MASTER)
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/master', false) >> 3
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo', false) >> 5
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/master', 4, false, CACHED_JOB_TTL_SECONDS)
0 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', 4, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/master', false) >> '3'
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo', false) >> '5'
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/master', '4', false, CACHED_JOB_TTL_SECONDS)
0 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', '4', false, CACHED_JOB_TTL_SECONDS)

buildPollingDelta.items.size() == 1
buildPollingDelta.items[0].branchedRepoSlug == 'test-org/test-repo/master'
buildPollingDelta.items[0].currentBuildNum == 4
buildPollingDelta.items[0].previousBuildNum == 3
buildPollingDelta.items[0].currentBuildNum == '4'
buildPollingDelta.items[0].previousBuildNum == '3'
}

void 'send events for build both on branch and on repository'() {
Expand All @@ -104,24 +104,25 @@ class TravisBuildMonitorSpec extends Specification {
then:
1 * travisService.getLatestBuilds() >> [ build ]
build.branchedRepoSlug() >> "test-org/test-repo/my_branch"
build.getNumber() >> 4
build.getNumber() >> '4'
build.getState() >> TravisBuildState.passed
build.jobs >> []
build.repository >> repository
repository.slug >> 'test-org/test-repo'

1 * travisService.getGenericBuild(build, true) >> TravisBuildConverter.genericBuild(build, MASTER)
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', false) >> 3
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/my_branch', 4, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', 4, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo', false) >> '-1'
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', false) >> '3'
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/my_branch', '4', false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', '4', false, CACHED_JOB_TTL_SECONDS)

1 * echoService.postEvent({
it.content.project.name == "test-org/test-repo"
it.content.project.lastBuild.number == 4
it.content.project.lastBuild.number == '4'
})
1 * echoService.postEvent({
it.content.project.name == "test-org/test-repo/my_branch"
it.content.project.lastBuild.number == 4
it.content.project.lastBuild.number == '4'
})
}

Expand All @@ -136,11 +137,12 @@ class TravisBuildMonitorSpec extends Specification {
then:
1 * travisService.getLatestBuilds() >> [ build ]
build.branchedRepoSlug() >> "test-org/test-repo/my_branch"
build.getNumber() >> 4
build.getNumber() >> '4'

1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', false) >> 3
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/my_branch', 4, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', 4, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo', false) >> '-1'
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', false) >> '3'
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/my_branch', '4', false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', '4', false, CACHED_JOB_TTL_SECONDS)

build.jobs >> []
build.repository >> repository
Expand All @@ -162,42 +164,43 @@ class TravisBuildMonitorSpec extends Specification {
then:
1 * travisService.getLatestBuilds() >> [ build, buildDifferentBranch ]
build.branchedRepoSlug() >> "test-org/test-repo/my_branch"
build.getNumber() >> 4
build.getNumber() >> '4'
build.getState() >> TravisBuildState.passed
build.jobs >> []
build.repository >> repository
buildDifferentBranch.branchedRepoSlug() >> "test-org/test-repo/different_branch"
buildDifferentBranch.getNumber() >> 3
buildDifferentBranch.getNumber() >> '3'
buildDifferentBranch.getState() >> TravisBuildState.passed
buildDifferentBranch.jobs >> []
buildDifferentBranch.repository >> repository
repository.slug >> 'test-org/test-repo'
1 * travisService.getGenericBuild(build, true) >> TravisBuildConverter.genericBuild(build, MASTER)
1 * travisService.getGenericBuild(buildDifferentBranch, true) >> TravisBuildConverter.genericBuild(buildDifferentBranch, MASTER)
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', false) >> 2
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/my_branch', 4, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', 4, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', 3, false, CACHED_JOB_TTL_SECONDS)
2 * buildCache.getLastBuild(MASTER, 'test-org/test-repo', false) >> '-1'
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', false) >> '2'
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/my_branch', '4', false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', '4', false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', '3', false, CACHED_JOB_TTL_SECONDS)

1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/different_branch', false) >> 1
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/different_branch', 3, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/different_branch', false) >> '1'
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/different_branch', '3', false, CACHED_JOB_TTL_SECONDS)

1 * echoService.postEvent({
it.content.project.name == "test-org/test-repo/my_branch" &&
it.content.project.lastBuild.number == 4
it.content.project.lastBuild.number == '4'
})
1 * echoService.postEvent({
it.content.project.name == "test-org/test-repo" &&
it.content.project.lastBuild.number == 4
it.content.project.lastBuild.number == '4'
})

1 * echoService.postEvent({
it.content.project.name == "test-org/test-repo" &&
it.content.project.lastBuild.number == 3
it.content.project.lastBuild.number == '3'
})
1 * echoService.postEvent({
it.content.project.name == "test-org/test-repo/different_branch" &&
it.content.project.lastBuild.number == 3
it.content.project.lastBuild.number == '3'
})
}

Expand All @@ -211,7 +214,7 @@ class TravisBuildMonitorSpec extends Specification {
build.branch = new V3Branch()
build.branch.name = "my_branch"
build.id = 1337
build.number = 4
build.number = '4'
build.state = TravisBuildState.started
build.jobs = []
build.repository = repository
Expand All @@ -233,18 +236,19 @@ class TravisBuildMonitorSpec extends Specification {
2 * travisService.getGenericBuild(_, _) >> { V3Build b, boolean fetchLogs ->
TravisBuildConverter.genericBuild(b, MASTER)
}
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', true) >> 3
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', false) >> 3
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/my_branch', 4, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', 4, false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo', false) >> '-1'
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', true) >> '3'
1 * buildCache.getLastBuild(MASTER, 'test-org/test-repo/my_branch', false) >> '3'
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo/my_branch', '4', false, CACHED_JOB_TTL_SECONDS)
1 * buildCache.setLastBuild(MASTER, 'test-org/test-repo', '4', false, CACHED_JOB_TTL_SECONDS)

1 * echoService.postEvent({
it.content.project.name == "test-org/test-repo"
it.content.project.lastBuild.number == 4
it.content.project.lastBuild.number == '4'
})
1 * echoService.postEvent({
it.content.project.name == "test-org/test-repo/my_branch"
it.content.project.lastBuild.number == 4
it.content.project.lastBuild.number == '4'
})
}
}