Skip to content

Conversation

@acornett21
Copy link
Contributor

@acornett21 acornett21 commented Mar 17, 2022

Issue #, if available:

Description of changes:
In order to get changes to code-generator made, the tests for OLM also need to be changed. Passing in the new env OPERATOR_SDK_BIN_PATH where necessary. Please let me know if I missed any places.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Signed-off-by: Adam D. Cornett adc@redhat.com

Comment on lines 41 to 42
- name: OPERATOR_SDK_BIN_PATH
value: "/usr/local/bin"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would prefer for this to be maintained inside the image. Since we are installing the binary as part of the Dockerfile, if we update that, we would also need to remember to update this Prow config. Would you mind just adding the export on a line underneath where we copy the operator SDK binary over? https://github.com/aws-controllers-k8s/test-infra/blob/main/prow/jobs/images/Dockerfile.olm-bundle-pr#L53-L54

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RedbackThomson I moved this change to the docker files, and also made these two docker files match where appropriate. LMK if there are any other issues.

Copy link
Contributor

@vijtrip2 vijtrip2 Mar 21, 2022

Choose a reason for hiding this comment

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

I am not sure if exporting the variable in a Dockerfile layer will make the env variable available during prow execution.

Why not add ENV OPERATOR_SDK_BIN_PATH=/usr/local/bin inside both Dockerfiles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vijtrip2 I wasn't sure either, that's why I added it to the jobs first, but you're probably right then need to be ENV's as well.

OLM_BUNDLE_VERSION=$(echo "$RELEASE_VERSION" | awk -F v '{print $NF}')
echo "olm-bundle-pr.sh][INFO] olm bundle version is $OLM_BUNDLE_VERSION"
export ACK_GENERATE_OLM=true
export OPERATOR_SDK_BIN_PATH=/usr/local/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

Echoing my comment below, but for this image too.

@acornett21 acornett21 force-pushed the add_operator_sdk_env branch from 9908f7f to 15b7dc6 Compare March 18, 2022 20:53
…e olm docker files

Signed-off-by: Adam D. Cornett <adc@redhat.com>
@acornett21 acornett21 force-pushed the add_operator_sdk_env branch from 15b7dc6 to c0e49c2 Compare March 21, 2022 19:43
@vijtrip2
Copy link
Contributor

/lgtm

I will build these new prow images, and publish them, and update the prow-jobs to use new image version, and then re-run the code-generator tests. :)

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2022
@ack-bot
Copy link
Collaborator

ack-bot commented Mar 21, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, vijtrip2

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

@ack-bot ack-bot merged commit a20db4c into aws-controllers-k8s:main Mar 21, 2022
@acornett21
Copy link
Contributor Author

Thanks @vijtrip2!!

@acornett21 acornett21 deleted the add_operator_sdk_env branch March 21, 2022 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants