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.

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

Revert "NO-ISSUE: Update go to 1.21.1 follow-up (#1966)" #2000

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

dsimansk
Copy link
Contributor

@dsimansk dsimansk commented Oct 4, 2023

This reverts commit 219907d.

Per my comment on: https://issues.redhat.com/browse/KOGITO-9799.

We should treat Go toolchain and go.mod min version as two separate entities. Because the latest Golang might not be available in the internal build systems etc.

@dsimansk dsimansk temporarily deployed to ci October 4, 2023 10:07 — with GitHub Actions Inactive
@dsimansk dsimansk temporarily deployed to ci October 4, 2023 10:07 — with GitHub Actions Inactive
@dsimansk dsimansk temporarily deployed to ci October 4, 2023 10:07 — with GitHub Actions Inactive
@caponetto caponetto requested a review from zdrapela October 4, 2023 10:10
Copy link
Contributor

@zdrapela zdrapela left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. I wasn't aware of that.

It looks good. Just so you know, check on this PR misses the IT/E2E tests in kn-plugin-workflow and doesn't run them. This will be fixed in PR #1992 (which depends on PR #1998 being merged first). It might be good to merge those first just to make sure.

@dsimansk
Copy link
Contributor Author

dsimansk commented Oct 4, 2023

Thanks for the fix. I wasn't aware of that.

It looks good. Just so you know, check on this PR misses the IT/E2E tests in kn-plugin-workflow and doesn't run them. This will be fixed in PR #1992 (which depends on PR #1998 being merged first). It might be good to merge those first just to make sure.

No worries, it's always a bit tricky with Go versions. I can share a few stories that have bitten us so far.

Anyway for the merge order, your proposal sounds good. But please note that there's a bit of time pressure to pull kn-plugin-workflow into kn for the next product build cycle. Depending really how much time the other 2 PRs will take.

@tiagobento tiagobento requested a review from ljmotta October 4, 2023 15:49
@tiagobento
Copy link
Contributor

Was discussing this with @ljmotta and our understanding is that this PR makes it possible to continue building kie-tools with any Go version higher than or equal to 1.20, is that correct?

@tiagobento tiagobento added area:core area:dependencies Pull requests that update a dependency file labels Oct 4, 2023
@dsimansk
Copy link
Contributor Author

dsimansk commented Oct 4, 2023

Was discussing this with @ljmotta and our understanding is that this PR makes it possible to continue building kie-tools with any Go version higher than or equal to 1.20, is that correct?

Yes, that's correct. That's the exact motivation for the PR. For that reason it isn't changing any Go version configured in GitHub action.

https://go.dev/doc/modules/gomod-ref#go

@dsimansk dsimansk temporarily deployed to ci October 4, 2023 21:22 — with GitHub Actions Inactive
@dsimansk dsimansk temporarily deployed to ci October 4, 2023 21:22 — with GitHub Actions Inactive
@dsimansk dsimansk temporarily deployed to ci October 4, 2023 21:22 — with GitHub Actions Inactive
@dsimansk dsimansk force-pushed the pr/revert-min-go-version branch from c23266a to 84577bd Compare October 5, 2023 07:30
@dsimansk
Copy link
Contributor Author

dsimansk commented Oct 5, 2023

@caponetto @zdrapela I've rebased the PR. Kindly asking for GH actions run approval, pls. :)

@dsimansk dsimansk temporarily deployed to ci October 5, 2023 10:10 — with GitHub Actions Inactive
@dsimansk dsimansk temporarily deployed to ci October 5, 2023 10:10 — with GitHub Actions Inactive
@dsimansk dsimansk temporarily deployed to ci October 5, 2023 10:10 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core area:dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants