-
Notifications
You must be signed in to change notification settings - Fork 816
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
[Release Automation] Update Helm/SDK/Install Packages Version Numbers #3149
Conversation
Build Failed 😱 Build Id: e5428ae0-c345-4a8f-a186-86bcc166806a To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
"strings" | ||
) | ||
|
||
func main() { |
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.
rather than the heavy duplication between after-release
and before-release
, I would recommend using a flag. For a script like this, no worries using the standard flag library: https://pkg.go.dev/flag
Examples in our repo include: https://github.com/googleforgames/agones/blob/main/examples/simple-game-server/main.go#L41-L49
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 have updated the code to make it a single script using Flag. Thanks!
|
||
func main() { | ||
if len(os.Args) < 2 { | ||
fmt.Println("Please provide the initial version as a command-line argument") |
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.
log.Fatalf here
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.
used log instead fmt package
for _, filename := range files { | ||
err := UpdateVersionInFile(filename, initialVersion) | ||
if err != nil { | ||
fmt.Printf("Error updating file %s: %s\n", filename, err.Error()) |
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.
maybe a log.Fatalf
instead of just a Printf?
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.
used log instead fmt
// Check the file extension and update version accordingly | ||
ext := filepath.Ext(filename) | ||
switch ext { | ||
case ".yaml", ".yml", ".nuspec", ".csproj": |
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'm not sure this needs a case statement. It seems like you can just write it as
if ext := filepath.Ext(filename); ext == ".json" {
updateJSONVersion()
} else {
updateVersion()
}
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 tried to update the JSON files without a switch/case statement, but the syntax was slightly different. So created a switch/case statement to ensure that the files were updated correctly.
return re.ReplaceAllString(content, newVersion+"-dev") | ||
} | ||
|
||
func updateJSONVersion(content string, initialVersion string) string { |
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.
It seems like the only difference here is handling "1.2.3"
vs 1.2.3
, is that right?
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 that's correct
Build Failed 😱 Build Id: 9c658717-ef7c-41d8-8ede-2d663a72f058 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 984ac6f7-e2ce-49d7-9584-8f392c4d1419 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: cb3cfbff-b8ad-433b-8a9d-d371a77a434a To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 03a4e914-e855-4ee8-81d3-f9352898c069 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
build/scripts/version-update/main.go
Outdated
var releaseType string | ||
|
||
func init() { | ||
flag.StringVar(&releaseType, "type", "", "Specify the release type ('before' or 'after')") |
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.
Maybe release-stage
instead of type
(and change variables)? releaseType
seems like it should be what type of release it is, vs what stage the release is in.
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.
Renamed type to release-stage and releaseType to releaseStage.
build/scripts/version-update/main.go
Outdated
return | ||
} | ||
|
||
releaseType := os.Args[1] |
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.
The way you are parsing this right now seems to ignore the flags - instead it looks like you would execute it via version-update before 1.2.3
, vs version-update --type=before 1.2.3
.
Since you are using the flag
package, you should trust that the flag
package can parse. For non-flag arguments, you should use flag.Args .. so line 23 should be if len(flag.Args()) < 1
, for instance.
releaseType
is already initialized in init()
above, so you don't need this line at all, and the line below should just be initialVersion := flag.Args()[0]
.
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 have updated the code and passed the --release-stage=before flag from the command line.
build/scripts/version-update/main.go
Outdated
} | ||
} | ||
|
||
func UpdateValueInFileBeforeRelease(filename string, valueToUpdate string) error { |
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.
There is a lot of duplication in the BeforeRelease
and AfterRelease
variants - you should be able to factor these together. Consider that all this really is is "replace regexp with value" and what you need is a function that takes a regexp and replaces - the regexp and replacement value are the parameters. Then you can set those parameters based on whether it's before/after and whether it's JSON, which seems to be the only differences.
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 tried my best to avoid the duplication😀
Please let me know if there are any areas of the code that you think could be improved. Thanks!
Build Failed 😱 Build Id: 2c613159-cd04-4be1-bb77-8941b9105be0 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: f6f49340-babd-4b6d-8c19-96006cd9cc62 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
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.
Getting close!
build/scripts/version-update/main.go
Outdated
content := string(fileBytes) | ||
|
||
ext := filepath.Ext(filename) | ||
validExtensions := []string{".yaml", ".yml", ".nuspec", ".csproj"} |
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 would eliminate this and just change the below to if ext == ".json"
- you're adding some amount of complexity just to validate a list that is more or less a constant, 20 lines previous to this.
Eliminating this will allow you to eliminate the logic in lines 62-72 and 86-90 as well - it seems like we're going through a lot of effort there.
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.
Updated the code and it looks simple now. Thanks Zach!
Build Failed 😱 Build Id: 778fe745-90c0-46ee-9321-08d56ef13e73 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Kalaiselvi84, zmerlynn The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
build/scripts/version-update/main.go
Outdated
|
||
import ( | ||
"flag" | ||
"io/ioutil" |
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.
linter failed: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
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.
replaced io/ioutil with os👍🏻
New changes are detected. LGTM label has been removed. |
Build Failed 😱 Build Id: bec7bfa5-ac87-4c34-938e-ece6bca0b1d6 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: ebe0670a-d4c4-4a86-9337-1a132e9bb991 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: cd58622f-90cf-4fa8-a8e0-ee32a53d098b To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Succeeded 👏 Build Id: 99bed0af-37ae-461a-8087-e0f8d9703a53 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
What type of PR is this?
/kind feature
What this PR does / Why we need it:
Scripts to increment version numbers in helm/sdk/install packages before and after release.
Which issue(s) this PR fixes:
Closes #2456
Special notes for your reviewer: