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

tests: Remove a bad FROM line #5291

Merged
merged 1 commit into from Jan 27, 2024

Conversation

edsantiago
Copy link
Collaborator

Some new heredoc test added "FROM blah blah python whatever",
an image that (presumably) exists on docker.io but does not
exist in our cache.

Plus, test was completely broken anyway. It would've found
the "this is the output" lines even without python, as
part of the verbose build.

Solution: don't use python. You don't need python to test a shebang.
You can use anything. 'cat' is traditional, but I choose 'rev'
because that makes it nearly impossible for the test to match
merely due to a build-step echo.

Signed-off-by: Ed Santiago santiago@redhat.com

None

Some new heredoc test added "FROM blah blah python whatever",
an image that (presumably) exists on docker.io but does not
exist in our cache.

Plus, test was completely broken anyway. It would've found
the "this is the output" lines even without python, as
part of the verbose build.

Solution: don't use python. You don't need python to test a shebang.
You can use anything. 'cat' is traditional, but I choose 'rev'
because that makes it nearly impossible for the test to match
merely due to a build-step echo.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago
Copy link
Collaborator Author

Caught by podman-buildah treadmill

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

Test was added by me @edsantiago 😅 , my intention was to just test if other interpreted languages can directly run code from containerfile.

But I agree rev is a cleaner and low-footprint method to test this, thank you so much for fixing this 😄

Copy link
Contributor

openshift-ci bot commented Jan 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, flouthoc

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

@flouthoc
Copy link
Collaborator

flouthoc commented Jan 25, 2024

@edsantiago If i got your second concern right for the build-step echo part I don't think that buildah prints heredoc content in the build output so everything between the heredoc content is just executed it is not printed on the build step output if that is what you were talking about.

@edsantiago
Copy link
Collaborator Author

I don't think that buildah prints heredoc content in the build output so everything between the heredoc content is just executed it is not printed on the build step output if that is what you were talking about.

It is, but I'm much more paranoid than you :-). If buildah is working properly, heredoc content is not printed. I can easily envision many forms of breakage in which heredoc is just passed through. Usually these would be errors, but I can also envision a missing error exit. And in that case:

RUN <<EOF
hi there
EOF

if a test only does a substring lookup for "hi there", it would pass. That's why as a general rule I always require tests to do some sort of text transform: rev, split or join or math formula or something. It's a good habit that has saved us on many occasions.

Great to hear from you! I hope your new adventures are treating you well!

print('this is the output of test12')
#!/usr/bin/env rev
11tset fo tuptuo eht si siht
21tset fo tuptuo eht si siht
Copy link
Member

Choose a reason for hiding this comment

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

For a brief dyslexic moment, I thought you had dropped a naughty word at the end of these lines....

@TomSweeneyRedHat
Copy link
Member

LGTM
and happy greeen tests

@rhatdan
Copy link
Member

rhatdan commented Jan 27, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 27, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit da1bdeb into containers:main Jan 27, 2024
34 checks passed
@edsantiago edsantiago deleted the remove_bad_from branch January 27, 2024 14:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants