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

NETOBSERV-505 NOO fails to build with go1.18 and above #146

Merged
merged 2 commits into from
Aug 5, 2022

Conversation

stleerh
Copy link
Contributor

@stleerh stleerh commented Aug 3, 2022

Use 'go install' to install the executable. This stopped working in
go1.18 because of https://go.dev/doc/go-get-install-deprecation.

In making this change, kustomize broke and no longer installed. The problem
is documented in kubernetes-sigs/kustomize#3618.
The solution is to use at least v4.5.2. kustomize was upgraded to 4.5.7
from 3.8.8 and everything still worked.

Use 'go install' to install the executable.  Note that it still calls
'go get' to update dependencies in go.mod.  This stopped working in
go1.18 because of https://go.dev/doc/go-get-install-deprecation.

In making this change, kustomize broke and no longer installed.  The problem
is documented in kubernetes-sigs/kustomize#3618.
The solution is to use at least v4.5.2.  kustomize was upgraded to 4.5.7
from 3.8.8 and everything still worked.
@jotak
Copy link
Member

jotak commented Aug 4, 2022

+1 but we need to make sure this major upgrade of kustomize doesn't have any wrong side effect on building bundle

@stleerh
Copy link
Contributor Author

stleerh commented Aug 4, 2022

+1 but we need to make sure this major upgrade of kustomize doesn't have any wrong side effect on building bundle

I tested the other make targets that used kustomize, and it seems fine.  However, I dd not see the failure that prow reported on controller-gen where it "cannot query module due to -mod=vendor".  May have broken backwards-compatibility.

There is also no need to update dependencies since it is installing a
specific version of an executable.
@stleerh
Copy link
Contributor Author

stleerh commented Aug 4, 2022

/retest

1 similar comment
@stleerh
Copy link
Contributor Author

stleerh commented Aug 4, 2022

/retest

Copy link
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @stleerh

@openshift-ci openshift-ci bot added the lgtm label Aug 5, 2022
@jotak
Copy link
Member

jotak commented Aug 5, 2022

/retest

@jotak
Copy link
Member

jotak commented Aug 5, 2022

/approve

@openshift-ci
Copy link

openshift-ci bot commented Aug 5, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jotak

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

@openshift-ci openshift-ci bot added the approved label Aug 5, 2022
@openshift-ci openshift-ci bot merged commit 8189b11 into netobserv:main Aug 5, 2022
KalmanMeth pushed a commit to KalmanMeth/network-observability-operator that referenced this pull request Feb 13, 2023
* Add recent_op_value and recent_count

* Update Counter's source

* Add integration test between extractor and prom encoder

* rename fields

* Update a comment

* Update network_definitions

* Address Eran's comment: Panic in initValue() on unknown operation

* Address Eran's comment: add prefix "total" to count and value

* Address Eran's comment: change struct member to be unexported

* Address Eran's comment: rename and move common test func to test/utils
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants