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

[th/hack-env-check] hack/env.sh: move checking of environment variables outside SKIP_VAR_SET block #819

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thom311
Copy link
Contributor

@thom311 thom311 commented Dec 19, 2024

When "$SKIP_VAR_SET" is unset and the environment variables fallback to the default, the check for valid values should be done. Move the check out of the $SKIP_VAR_SET block for that.

For the current "hack/env.sh" this maybe not make an actual difference, because probably the code to assign default values will ensure that always valid value are set. Note that the openshift variant of the above code will detect the default via skopeo, which can fail. For that reason, this change makes more sense for openshift. However, also for the current code, performing the same error checking after filling out default values, ensures that the detected values are considered valid Even if that is in fact always the case, it's not entirely trivial to see.


A follow up will later be for openshift here.

Code like

        CNI_IMAGE_DIGEST=$(skopeo inspect docker://quay.io/openshift/origin-sriov-cni | jq --raw-output '.Digest')
        export SRIOV_CNI_IMAGE=${SRIOV_CNI_IMAGE:-quay.io/openshift/origin-sriov-cni@${CNI_IMAGE_DIGEST}}

is wrong, because skopeo inspect docker://quay.io/openshift/origin-sriov-cni command will fail. The first problem is that that failure will fail the entire script (bash -e). The second problem is that the detected value is clearly invalid. This should be dealt with.

However, I think the bash script should first also move the error checking out of the if [ -z $SKIP_VAR_SET ]; then ... else block.

…SET block

When  "$SKIP_VAR_SET" is unset and the environment variables fallback to
the default, the check for valid values should be done. Move the check
out of the $SKIP_VAR_SET block for that.

For the current "hack/env.sh" this maybe not make an actual difference,
because probably the code to assign default values will ensure that
always valid value are set. Note that the openshift variant of the above
code will detect the default via skopeo, which can fail. For that
reason, this change makes more sense for openshift. However, also for
the current code, performing the same error checking after filling out
default values, ensures that the detected values are considered valid
Even if that is in fact always the case, it's not entirely trivial to
see.
Copy link

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 12411815934

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.007%) to 47.231%

Files with Coverage Reduction New Missed Lines %
controllers/drain_controller.go 4 77.68%
Totals Coverage Status
Change from base Build 12395829910: -0.007%
Covered Lines: 7199
Relevant Lines: 15242

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants