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

test: Add checkout kbs helper script #2105

Merged

Conversation

stevenhorsman
Copy link
Member

We have duplicate code in multiple e2e workflows
and manual doc instructions for checking out the KBS repo and switching to the correct branch. This both makes updating it prone to errors/omissions and means that if we make changes we can't test them in PRs due to the
pull_request_target limitation, so split these out into a single script we can call from elsewhere.

@stevenhorsman
Copy link
Member Author

@mkulke - based on @wainersm's suggestion to do the KBS checkout in a nicer way I've created this PR. I was going to wait for your caching PR to be merged first, as there is some code overlap, but I figured that you might find it useful to have the some of the changes centralised and moved out of the workflow, so they can be updated easier and tested without the pull_request_target issues, so I will leave the ordering decision up to you. I'm leaving it as draft to be on the safe side for now. Thanks!

#
# SPDX-License-Identifier: Apache-2.0

TEST_DIR=$(cd "$(dirname "$(realpath "$0")")/../" || exit; pwd)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this idiom. this is a subshell, so if we can't go into the folder we exit from the subshell? don't we want to abort with an error if any of those operations fail (i.e. set -euo pipefail` at the top)?

Copy link
Member Author

@stevenhorsman stevenhorsman Oct 10, 2024

Choose a reason for hiding this comment

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

Yeah sorry - shellcheck was nagging me about it and I probably added it without my brain engaged here. set -euo pipefail is the better approach, so I can remove a whole bunch of the || exit now. Thanks!

Comment on lines 35 to 36
popd || exit
popd || exit
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess at the end of the script we don't have to popd. A bash script will run its own pwd.

@mkulke
Copy link
Contributor

mkulke commented Oct 10, 2024

@mkulke - based on @wainersm's suggestion to do the KBS checkout in a nicer way I've created this PR. I was going to wait for your caching PR to be merged first, as there is some code overlap, but I figured that you might find it useful to have the some of the changes centralised and moved out of the workflow, so they can be updated easier and tested without the pull_request_target issues, so I will leave the ordering decision up to you. I'm leaving it as draft to be on the safe side for now. Thanks!

makes sense, I don't mind rebasing

@stevenhorsman stevenhorsman force-pushed the rework-kbs-checkout branch 2 times, most recently from 1e5172d to c2f596a Compare October 10, 2024 16:34
@stevenhorsman stevenhorsman marked this pull request as ready for review October 10, 2024 16:34
@stevenhorsman stevenhorsman requested a review from a team as a code owner October 10, 2024 16:34
We have duplicate code in multiple e2e workflows
and manual doc instructions for checking out the KBS
repo and switching to the correct branch. This both makes
updating it prone to errors/omissions and means that if we
make changes we can't test them in PRs due to the
`pull_request_target` limitation, so split these out into a
single script we can call from elsewhere.

Signed-off-by: stevenhorsman <[email protected]>
Add sudo to install and ensure it has execute permission
Copy link
Member

@wainersm wainersm left a comment

Choose a reason for hiding this comment

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

Thanks for listening my dreams! it simplies a lot for local and CI runs, remove duplicates... great!

@wainersm wainersm merged commit 191ec51 into confidential-containers:main Oct 10, 2024
18 of 19 checks passed
@stevenhorsman stevenhorsman deleted the rework-kbs-checkout branch October 10, 2024 18:11
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.

3 participants