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

Move to go1.19 #96

Merged
merged 4 commits into from
Feb 15, 2023
Merged

Move to go1.19 #96

merged 4 commits into from
Feb 15, 2023

Conversation

jotak
Copy link
Member

@jotak jotak commented Feb 13, 2023

No description provided.

@openshift-ci
Copy link

openshift-ci bot commented Feb 13, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jotak
Copy link
Member Author

jotak commented Feb 13, 2023

Moving away from the go-toolset image as it often lays some versions behind

@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2023 •
edited

Codecov Report

Merging #96 (d557fdf) into main (c54e7eb) will not change coverage.
The diff coverage is 0.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main      #96   +/-   ##
=======================================
  Coverage   41.54%   41.54%           
=======================================
  Files          29       29           
  Lines        1993     1993           
=======================================
  Hits          828      828           
  Misses       1127     1127           
  Partials       38       38
Flag Coverage Δ
unittests 41.54% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
e2e/cluster/kind.go 22.91% <0.00%> (ø)
e2e/cluster/tester/loki.go 0.00% <0.00%> (ø)
pkg/agent/tls.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

msherif1234
msherif1234 reviewed Feb 14, 2023
View reviewed changes
Dockerfile
@@ -1,8 +1,7 @@
# Build the manager binary
FROM registry.access.redhat.com/ubi9/go-toolset:1.18.4 as builder
FROM docker.io/library/golang:1.19 as builder
Copy link
Contributor

@msherif1234 msherif1234 Feb 14, 2023 •
edited

Choose a reason for hiding this comment

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

is this intentional ? we should find go1.19 go-toolset from redhat registry right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it is intentional, ubi's go-toolset often lies behind in terms of go version so we want something more up to date.
This is for the upstream/community build, so using golang:1.19 is fine (we actually do the same already for a while in flowlogs-pipeline); the Dockerfile for downstream is different and uses images from redhat registry.

msherif1234
msherif1234 reviewed Feb 14, 2023
View reviewed changes
Copy link
Contributor

@msherif1234 msherif1234 left a comment

Choose a reason for hiding this comment

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

pls restore changes to bpf_bpfel/g.go files no need to merge them
I have a question about new builder image

@msherif1234
Copy link
Contributor

Moving away from the go-toolset image as it often lays some versions behind

is that considered secure from RH prespective ? did u check to see if there are other alternatives ?

@jotak
Copy link
Member Author

jotak commented Feb 14, 2023

pls restore changes to bpf_bpfel/g.go files no need to merge them I have a question about new builder image

it wasn't a manual edit, it's a new format from go fmt (when using go 1.19), so even though they are generated, I guess they would still adopt that format if re-generated

@jotak jotak added no-doc This PR doesn't require documentation change on the NetObserv operator no-qe This PR doesn't necessitate QE approval labels Feb 14, 2023
@msherif1234
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 14, 2023
@jotak jotak merged commit 358f76b into netobserv:main Feb 15, 2023
shach33 pushed a commit to praveingk/netobserv-ebpf-agent that referenced this pull request Apr 6, 2023
@jotak @shach33
Move to go1.19 (netobserv#96)
e5888db 
* Move to go1.19

* Fix go1.19 breaking cvhanges

* Still allow building with 1.18

* revert space changes on autogen files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msherif1234 msherif1234 msherif1234 left review comments

@OlivierCazade OlivierCazade OlivierCazade approved these changes

@praveingk praveingk Awaiting requested review from praveingk

Assignees

@msherif1234 msherif1234

@OlivierCazade OlivierCazade

Labels
lgtm no-doc This PR doesn't require documentation change on the NetObserv operator no-qe This PR doesn't necessitate QE approval
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jotak @codecov-commenter @msherif1234 @OlivierCazade