-
Notifications
You must be signed in to change notification settings - Fork 64
🌱 fix(ci): sanitize env for asciinema upload #1925
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
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
7cc2f10
to
b185c92
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1925 +/- ##
==========================================
- Coverage 66.06% 66.01% -0.05%
==========================================
Files 70 70
Lines 6182 6182
==========================================
- Hits 4084 4081 -3
- Misses 1839 1841 +2
- Partials 259 260 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
69de856
to
9062824
Compare
On an unrelated note, is there a reason why we are regenerating this ascii demo as a pre-submit job, which could easily break things? Why not a post-submit job? |
9062824
to
09a8c4b
Compare
Change script that generates demo to cleanup envvars to avoid issue faced in the CI to upload the records.
09a8c4b
to
3df2b97
Compare
PATH="$PATH" \ | ||
TERM="xterm-256color" \ | ||
SHELL="/bin/bash" \ | ||
make demo-update |
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.
Hi @tmshort
(Yes, I know env -i starts with an empty environment. Do you know what's in the existing CI's environment that's causing a problem?)
The fix keeps only the required ENV VARs, so future GitHub changes won’t break anything. For example, I couldn’t reproduce the issue locally (macOS) with the current code.
I didn’t dive deep into the exact ENV differences**—Because today it’s ENV A, tomorrow ENV B.** The goal was just to unblock us and avoid this from happening again. If we want to rethink the approach, we can always do that in a future PR. Personally, I don’t think it’s worth spending more time here on this one.
Let me know if anything seems off!
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 works locally for me on linux... but the question is, why is the CI environment "off"?
I would at least like us to consider that this should be a post-submit job, and that it shouldn't block PRs... (not that it does right now) |
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.
/lgtm
(To at least avoid the red ❌ )
02fa296
This has not fixed the Ci... :( |
Description
Fix the issue:
Example: https://github.com/operator-framework/operator-controller/actions/runs/14495113504/job/40670980647?pr=1924
Reviewer Checklist