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

Configure User Agent #75

Merged
merged 5 commits into from Aug 10, 2023
Merged

Configure User Agent #75

merged 5 commits into from Aug 10, 2023

Conversation

stephenfin
Copy link
Contributor

This can be a helpful breadcrumb when debugging services.

Note
Unlike openshift/openstack-cinder-csi-driver-operator#123 and
openshift/csi-driver-manila-operator#191, we need to
create the version package here and modify our Makefile to configure
correct version attributes.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2023
mandre
mandre reviewed Aug 1, 2023
View reviewed changes
Copy link
Member

@mandre mandre left a comment

Choose a reason for hiding this comment

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

It doesn't look like it's working. From the logs, we're not getting the commit sha:

/var/log/containers/httpd/nova-api/nova_api_wsgi_access.log.1:192.168.24.1 - - [31/Jul/2023:17:51:32 +0000] "GET /v2.1/servers/a83db7c2-594e-411e-bf60-a55561ebed9f HTTP/1.1" 200 885 "-" "machine-api-provider-openstack/ gophercloud/v1.5.0"

Probably because of how we build the image.

@mandre
Copy link
Member

mandre commented Aug 1, 2023

We're not using the makefile to build the binary in the container image: https://github.com/openshift/machine-api-provider-openstack/blob/master/Dockerfile.rhel#L5

We need to change that, similarly to what I did in openshift/csi-driver-manila-operator#194

@stephenfin
Copy link
Contributor Author

/hold while we rework the Dockerfile to make use of make

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2023
@stephenfin
Copy link
Contributor Author

Dockerfile rework is at #76

@mandre
Copy link
Member

mandre commented Aug 1, 2023

I believe you'll need to rebase for #76 to take effect.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2023
@stephenfin
Copy link
Contributor Author

/unhold

#76 is merged and this PR is rebased

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2023
@stephenfin
Copy link
Contributor Author

/retest-required

@stephenfin
Copy link
Contributor Author

Whelp, still not seeing versions set correctly.

$ zgrep machine-api-provider-openstack /var/log/containers/httpd/nova-api/*
...
/var/log/containers/httpd/nova-api/nova_api_wsgi_access.log.7.gz:192.168.24.1 - - [02/Aug/2023:12:50:32 +0000] "GET /v2.1/servers/ed12702d-1300-447a-95ac-373e77cdad50 HTTP/1.1" 200 953 "-" "machine-api-provider-openstack/ gophercloud/v1.5.0"
...

More work to do here, clearly.

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2023
@stephenfin
Add version package
b1b7cac 
Based on the cluster-api-provider-openstack implementation but with all
the tag-related stuff stripped out since we don't tag our releases.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
@stephenfin
Configure User Agent
848dc07 
This can be a helpful breadcrumb when debugging services.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
@stephenfin
nit: Rename variable
f7cf2e2 
Consistency.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
@stephenfin
Add '-version' option
2d84c5e 
Also useful for debugging.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
@stephenfin
gitignore: Ignore built binaries
f9c36c3 
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
@stephenfin
Copy link
Contributor Author

/unhold

I was using the wrong module path 🙈

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2023

@stephenfin: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-openstack-nfv-intel f9c36c3 link false /test e2e-openstack-nfv-intel

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@stephenfin
Copy link
Contributor Author

Okay, this is working now:

$ zgrep machine-api-provider-openstack /var/log/containers/httpd/nova-api/* 
...
/var/log/containers/httpd/nova-api/nova_api_wsgi_access.log.1:192.168.24.1 - - [09/Aug/2023:13:35:30 +0000] "GET /v2.1/servers/51eeee10-0ca6-4308-9c92-fa7ab0073ff5 HTTP/1.1" 200 955 "-" "machine-api-provider-openstack/3f71a8d6ec229acbe5e6cd719c6aaec306577ac8 gophercloud/v1.5.0"
...

🥳

mandre
mandre reviewed Aug 10, 2023
View reviewed changes
Copy link
Member

@mandre mandre left a comment

Choose a reason for hiding this comment

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

Great!
/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 10, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandre, stephenfin

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-merge-robot openshift-merge-robot merged commit cd7f2c1 into openshift:master Aug 10, 2023
8 of 9 checks passed
@stephenfin stephenfin deleted the add-user-agent branch August 10, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mandre mandre mandre left review comments

@mdbooth mdbooth Awaiting requested review from mdbooth

@pierreprinetti pierreprinetti Awaiting requested review from pierreprinetti

Assignees

@mandre mandre

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@stephenfin @mandre @openshift-merge-robot