Bug 1881907: Adds cluster version into the User-Agent header

[0sewa0]

No description provided.

[tisnik]

It looks ok (giving the ? string is expected by consumers)

[0sewa0]

/retest

[0sewa0]

/retest

[0sewa0]

/retest

[rluders]

lgtm

[0sewa0]

/retest

[iNecas]

Looks good codewise but haven't a chance to try out: did anyone of the reviewers tried running this end2end?

[0sewa0]

Don't know

[iNecas]

Ok, I've tested via cluste-bot, as well as locally, and have seen the archvie getting though end2end, with the changes, so looks good from this perspective.

Code-wise, I know it follows an already established pattern, where the gatherer loads some info about the cluster and the client uses it when uploading the data. Said that, I don't like that approach a lot, as it couples the gathering and uploading in some unexpected way (what would happen if we had a configuration to not gather the cluster operators data).

Therefore, I'm good with acking this, as long as we have some refactoring task to decouple getting data for preparing the client from the generic gathering.

[0sewa0]

The ClusterVersion is also from the gathering part, so I basically followed that pattern. IMO probably most of insights operator should be refactored, or at least the gathering part.
Few examples why:

• Working in a file that is more than 1200 lines long is not fun. (also its named clusterconfig.go why ??)
• Having i as the reference to the Gatherer object in methods is inconvenient.

[martinkunc]

Good work, thank you!

Woudln't it be better to use RELEASE_VERSION env variable, which doesn't read value from cluster, but from Pod actually running ?

Maybe we could refactor the expression to method to return UserAgent value, together with waiting for ClusterVersion.
Also, I believe we could create the whole UserAgent string in semver form like:

func UserAgent(releaseVersionEnv, v apimachineryversion.Info, cv ClusterVersion) string
gitVersion := v.GitVersion
// Because we dont set GitVersion, just GitCommit, use kubernetes machinery, but fallback to RELEASE_VERSION envvar.
if gitVersion == version.GitVersion  { // default, not overwritten by build
  gitVersion = fmt.Sprintf("%s-%s", releaseVersionEnv, v.GitCommit)
}
return fmt.Sprintf("insights-operator/%s-%s cluster/%s",  gitVersion, cv.Spec.ClusterID)
}

[0sewa0]

@iNecas what do you think about ^ ?

[0sewa0]

/retest

[0sewa0]

@iNecas @martinkunc please review :)

[openshift-ci-robot]

@0sewa0: This pull request references Bugzilla bug 1881907, which is invalid:

• expected the bug to target the "4.6.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1881907: Adds cluster version into the User-Agent header

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/test-infra repository.

[0sewa0]

/bugzilla refresh

[openshift-ci-robot]

@0sewa0: This pull request references Bugzilla bug 1881907, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target release (4.6.0) matches configured target release for branch (4.6.0)
• bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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/test-infra repository.

[iNecas]

Have you tried with real cluster, when trying against one, I'm getting v0.0.0-master+$Format:%h$-$Format:%H$ in the gitVersion, which doesn't look right.

[0sewa0]

How can I try it on a 'real cluster' ? I always run it locally while pointing at a cluster created by cluster-bot

[0sewa0]

So after a bit of investigation the build command doesn't seem to work as intended. The values the version returns are the defaults for that package and not the special ones that we should be setting during build.

build:
go build -ldflags "-X github.com/openshift/insights-operator/vendor/k8s.io/client-go/pkg/version.gitCommit=$$(git rev-parse HEAD) -X github.com/openshift/insights-operator/vendor/k8s.io/client-go/pkg/version.gitVersion=v1.0.0+$$(git rev-parse --short=7 HEAD)" -o bin/insights-operator ./cmd/insights-operator

[martinkunc]

Yes, that is right. That is why this change prefers RELEASE_VERSION envvar. If the var is set, it overrides the value built-in the package. Setting it should then affect Debug as well.
At some point later we would also like to resolve filling version in the ldflags, but I would do it in a separate CL.

[0sewa0]

I tested by using the envvar, which works but it still appends the v.GitCommit to the the value of the envvar and that will be"$Format:%H$". So the gitVersion will be {RELEASE_VERSION} - $Format:%H$ the commit part of it not really helpful

[martinkunc]

When you build it with make build, it should set the gitVersion in the binary, when you run the binnary afterwards, will it have it set right ? It should.

[0sewa0]

conclusion: the make build had incorrect path to the packages where it tried to set the variables.

incorrect path: github.com/openshift/insights-operator/vendor/k8s.io/client-go/pkg/version.gitCommit
correct path: k8s.io/client-go/pkg/version.gitCommit

[martinkunc]

Thank you, Marcell, good work.
/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0sewa0, martinkunc, rluders, tisnik

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:

• OWNERS [0sewa0,martinkunc,rluders,tisnik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[0sewa0]

/retest

[0sewa0]

/retest

[openshift-ci-robot]

@0sewa0: All pull requests linked via external trackers have merged:

• openshift/insights-operator#175

Bugzilla bug 1881907 has been moved to the MODIFIED state.

In response to this:

Bug 1881907: Adds cluster version into the User-Agent header

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/test-infra repository.