-
Notifications
You must be signed in to change notification settings - Fork 91
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
Upgrade devfile registry testing modules & CI to Go 1.21 #420
Conversation
Skipping CI for Draft Pull Request. |
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.
Shouldn't we update the workflows too?
registry/.github/workflows/ci.yaml
Line 39 in f974b36
go-version: 1.19 - (Not sure about this one)
go-version: 1.13 go-version: "1.19" go-version: "1.19"
tests/odov3/go.mod
Outdated
@@ -1,6 +1,6 @@ | |||
module github.com/devfile/registry/tests/odov3 | |||
|
|||
go 1.19 | |||
go 1.21 | |||
|
|||
require ( | |||
github.com/devfile/api/v2 v2.2.2 |
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.
might worth upgrading the devfile/api too?
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 is why I left this PR in WIP / blocked, need devfile/library#212 to merge before I can update devfile/api since devfile/library is a direct dependency as well.
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 is why I left this PR in WIP / blocked, need devfile/library#212 to merge before I can update devfile/api since devfile/library is a direct dependency as well.
Oh I see its merged now, I'll update the dependencies today.
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.
Also I didn't notice while reviewing that the PR was actually draft :D
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.
Just waiting for openshift/release#53126 now.
Signed-off-by: Michael Valdron <[email protected]>
e889d7b
to
ff82b5a
Compare
Signed-off-by: Michael Valdron <[email protected]>
/hold waiting for openshift/release#53126 to be merged. |
/unhold openshift/release#53126 is merged now |
/retest all |
@michael-valdron: The
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/test all |
@@ -39,7 +39,6 @@ var stacksPath string | |||
var stackDirs string | |||
|
|||
func init() { | |||
rand.Seed(time.Now().UnixNano()) |
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 understand we removed this due to it being deprecated now in our updated Go version but did it serve an important purpose? Should we be looking into using the suggested change 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.
@Jdubrick From my understanding, this is why I removed it:
If Seed is not called, the generator is seeded randomly at program startup.
Before 1.20, we needed to call rand.Seed(time.Now().UnixNano())
to generate a random seed, since 1.20 this is done at startup therefore we do not need to do this anymore. NewRand(NewSource(seed))
is for setting deterministic seeds with a scoped random generator.
@@ -19,7 +17,6 @@ var stacksPath string | |||
var stackDirs string | |||
|
|||
func init() { | |||
rand.Seed(time.Now().UnixNano()) |
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.
Same as other comment
/retest |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jdubrick, michael-valdron, thepetk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does this PR do?:
Summarize the changes. Are any stacks or samples added or updated?
Summary of Updates:
Which issue(s) this PR fixes:
Link to github issue(s)
fixes devfile/api#1558
PR acceptance criteria:
Have you read the devfile registry contributing guide and followed its instructions?
Does this repository's tests pass with your changes?
Does any documentation need to be updated with your changes?
Have you tested the changes with existing tools, i.e. Odo, Che, Console? (See devfile registry contributing guide on how to test changes)
How to test changes / Special notes to the reviewer: