-
Notifications
You must be signed in to change notification settings - Fork 202
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
[skip-ci] TMT: revdep podman build test #1892
base: main
Are you sure you want to change the base?
Conversation
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
1 similar comment
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
a7c3e39
to
815cd3a
Compare
@Luap99 @edsantiago @TomSweeneyRedHat PTAL. Still in draft and not ready for merge either way, but if this looks ok for starters, I'd like further comments on the test matrix to enable. Right now it's only fedora-rawhide-x86_64. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not so sure how much value a build test adds, IMO build breaks are generally known by the submitter/reviewer. I guess it adds some value to prevent accidental breakages but I don't think they are very common.
I am not against doing something like this but I think we should have proper discussion first. And as a PoC could you add a "broken" commit here to see how this would look like when it fails?
plans/podman_build_test.sh
Outdated
git clone https://github.com/containers/podman | ||
|
||
cd podman | ||
dnf -y builddep rpm/podman.spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this stuff be installed in the prepare step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be doable if we're only running podman tests. If we also want to do buildah, skopeo and anything else, then probably best done in individual tests.
plans/main.fmf
Outdated
summary: Build Podman | ||
execute: | ||
how: tmt | ||
script: bash ./plans/podman_build_test.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it's not directly related to building, running, or documenting podman perhaps this could live under ./contrib/tmtplans
or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will try with this change.
plans/podman_build_test.sh
Outdated
|
||
if [ -f /etc/fedora-release ]; then | ||
export TMPDIR=/var/tmp | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me (a lay person, non-tmt expert) why this is needed. Perhaps a comment would be helpful to other non-experts?
plans/podman_build_test.sh
Outdated
git add vendor/ | ||
git config --global user.email "[email protected]" | ||
git config --global user.name "Your Name" | ||
git commit -am 'add vendored files' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider grouping all the git repository setup steps together. It will make both the logs and the script easier to read and maintain.
It would be nice to get @edsantiago 's eyeballs on this if you have the time to wait. |
@@ -0,0 +1,24 @@ | |||
#!/usr/bin/env bash | |||
|
|||
set -eox pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set -eox pipefail | |
set -exo pipefail |
The order here is important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick first pass. Please also address Chris's comment.
if [ -f /etc/fedora-release ]; then | ||
export TMPDIR=/var/tmp | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the conditional? In all test environments I've seen, /var/tmp
is much bigger than /tmp
. It seems safest just to universally set TMPDIR=/var/tmp
.
contrib/tmtplans/.main.fmf.swp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oopsie
export TMPDIR=/var/tmp | ||
fi | ||
|
||
git clone https://github.com/containers/podman |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script is named buildah_build_test
but there's no buildah anything anywhere?
Now I understand the script names. Suggestion: rename both to "build_XXX" instead of XXX_build? "buildah_build_test," to me, suggests that you are testing buildah's build
command.
07b3064
to
792820c
Compare
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lsm5 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
1 similar comment
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
792820c
to
5421949
Compare
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
2 similar comments
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
5421949
to
ea36990
Compare
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
2 similar comments
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
Failed to load packit config file:
For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
1c8ce7d
to
0b6bf43
Compare
rhel copr targets are often outdated causing failed builds and tests. Best to only have CentOS Stream targets upstream, so we ensure everything that enters RHEL at some point has been tested upstream. Signed-off-by: Lokesh Mandvekar <[email protected]>
0b6bf43
to
d62b3be
Compare
Co-authored-by: Chris Evich <[email protected]> Signed-off-by: Lokesh Mandvekar <[email protected]>
d62b3be
to
ba5008d
Compare
No description provided.