-
Notifications
You must be signed in to change notification settings - Fork 53
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
Wizard: switch snapshot date in wizard to RFC3339 #2526
Conversation
Can one of the admins verify this patch? |
73cf6e6
to
d06b9d2
Compare
09ccdf0
to
3b57ae4
Compare
issue here |
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.
Thank you!
Could you edit the commit message? The PR title is good. https://osbuild.org/docs/developer-guide/general/workflow#preferred-commit-message-format
f7fee66
to
34b022e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## main #2526 +/- ##
==========================================
- Coverage 84.86% 84.86% -0.01%
==========================================
Files 183 183
Lines 20777 20783 +6
Branches 2016 2018 +2
==========================================
+ Hits 17633 17638 +5
- Misses 3122 3123 +1
Partials 22 22
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
c9606f1
to
723bb0f
Compare
@croissanne this is ready for another review when you have a chance! also, you mentioned image-builder needing an update to move from YYYY-MM-DD to RFC3339 too? let me know if there is anywhere else that will need a change as well, tracking them all here |
723bb0f
to
7774e4a
Compare
7774e4a
to
6535137
Compare
Looks like this has been stale for a bit, but it also seems to me that this should still be addressed, no? |
6535137
to
7c1982a
Compare
@ochosi agreed, this lets us remove some tech debt from content-sources. i believe there are some changes needed in image-builder and maybe IQE tests too? i'm not sure which would need to be merged first. CC @croissanne |
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.
Hey @xbhouse this looks great, thank you! pr_check is passing so IQE should be hopefully fine. @jrusz what do you think? Could changing the snapshot date format break something outside of pr_check? As for API, it looks like we're all set there: osbuild/image-builder#1252
We have a specific function to convert between the api and ui format in IQE plugin so it will most likely need some adjustment, that's going to be an easy change though |
061b400
to
972925b
Compare
@xbhouse Could you please squash the commits and rebase? The changes look great ✨ @jrusz @tkoscieln This will be good to merge, could you please take a look at what changes will be needed and let me know when they're ready? |
Let me catch up on this change and I'll make the adjustment in IQE tests |
972925b
to
2684aa5
Compare
/retest |
@xbhouse It seems this is not ready to merge yet. I tested it out and got a 500 code when trying to start the composes with a following error:
@regexowl produced a fix for it here in backend. |
ooh ok, nice catch! thank you ❤️ |
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
2684aa5
to
187f364
Compare
The backend fix got merged! Trying to compose an image and all looks good so far. @tkoscieln could you please take a look at the IQE test adjustment? |
@regexowl I tested it out, the compose works fine and I tried running the integration tests on it (as those are the ones that test the snapshotting) and they passed fine, so it seems this change is good to go. |
@tkoscieln Excellent, thank you! I'd say we're ready to merge then. |
187f364
to
94326cb
Compare
I believe everything was addressed.
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.
Thank you! ❤️
thank you all for your help on this!! i'm going to switch the date format to just RFC3339 in content-sources next (currently we're supporting both RFC3339 and YYYY-MM-DD), so once that's merged i'll monitor for any issues but hopefully everything goes smoothly 🙏 |
Thank you! Please let me know if anything seems amiss. |
The changes here should format the requested date from YYYY-MM-DD to RFC3339 in the snapshot step of the wizard. This helps content-sources clean up some tech debt and allows us to use RFC3339 for all date formats in our template-related APIs.
Background:
This PR removed support for YYYY-MM-DD in the request for
/snapshots/for_date/
and broke things. The snapshot step in the wizard needed to be converted to use RFC3339 also. This commit has been reverted and things are good again.Without these changes in the wizard, errors like this would be seen when calling
/snapshots/for_date/
from the content sources API with an unsupported date format: