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(artifact): more resilient app version parsing #948

Merged
merged 2 commits into from
Mar 31, 2020
Merged

fix(artifact): more resilient app version parsing #948

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41403c4
fix(artifact): more resilient app version parsing
emjburns Mar 31, 2020
35935a2
Merge branch 'master' into fix-hacky-app-version-parsing
luispollo Mar 31, 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
21 changes: 16 additions & 5 deletions keel-web/src/main/kotlin/com/netflix/spinnaker/keel/services/ApplicationService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,25 @@ class ApplicationService(
): ArtifactVersionSummary =
when (artifact.type) {
deb -> {
val appversion = AppVersion.parseName(version)
ArtifactVersionSummary(
var summary = ArtifactVersionSummary(
version = version,
environments = environments,
displayName = appversion?.version ?: version.removePrefix("${artifact.name}-"),
build = if (appversion != null) BuildMetadata(id = appversion.buildNumber.toInt()) else null,
git = if (appversion != null) GitMetadata(commit = appversion.commit) else null
displayName = version.removePrefix("${artifact.name}-")
)

// attempt to parse helpful info from the appversion.
// todo: replace, this is brittle
val appversion = AppVersion.parseName(version)
if (appversion?.version != null) {
summary = summary.copy(displayName = appversion.version)
Copy link
Contributor

@lorin lorin Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do we ever expect fields of appversion to be null during normal operation? If not, if this if check fails, I'd put a WARN level log statement with the value of theversion function argument to make it easier to debug in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we might fail, we might not fail if that's the case. it definitely puts us into a weird state. once we pull the appversion off the ami though it shouldn't be null. we don't rely on any of the other fields that i'm using here (if i remember correctly). Logging would probably be helpful, you're right.

}
if (appversion?.buildNumber != null) {
summary = summary.copy(build = BuildMetadata(id = appversion.buildNumber.toInt()))
}
if (appversion?.commit != null) {
summary = summary.copy(git = GitMetadata(commit = appversion.commit))
}
summary
Comment on lines +165 to +175
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm failing to see how this is different from what you had before, other than setting displayName outside this block above. I find this format harder to read than the one with the property names and inline if statements to the left. It's also potentially copying the whole object 3 times over which I know is not a huge deal but I feel if we just let these spread they may become a problem.

Copy link
Contributor

@lorin lorin Mar 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I believe the current problem is that appversion.buildNumber is null for one of the artifacts, so the appversion.buildNumber.toInt() call fails on line 162. In the new implementation, this call is guarded by a null chcek in line 169.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lorin is correct. Again, this code should be replaced in Q2.

}
docker -> {
var build: BuildMetadata? = null
Expand Down