-
Notifications
You must be signed in to change notification settings - Fork 2
feat: parse project toml files manually #5
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
base: main
Are you sure you want to change the base?
Conversation
1a90157
to
68ed38d
Compare
The release_schedule workflow should create the artifacts on this repository so other repositories can access it, and make sure we have a paper trail
Action should create a PR with updated dependencies. Target branch and tool to be used are configurable. Currently only pixi is supported but it's good to make it possible to add other tools down the line
1d00217
to
33c5e2f
Compare
Right, I think it's ready for review now. Smoothed out a few kinks, and made it self testing, which I think is kinda nice. Looking forward to hearing your feedback! |
@nabobalis Would you be able to share when you might have time to review the PR? I’ve got a few people I’d like to start using this solution, and I’m hoping to keep up the momentum. |
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 have not had time to run the code locally, but I am happy to merge and get it going and see what bugs we hit in a live environment.
Hi Sam, sorry about that. I went on vacation, so I just gave it a very quick once over and I am happy to merge and see how it works live. |
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.
There are a lot of review comments from both of us, so please don't merge it prematurely, I would rather get this right than fix in production.
schedule.json
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.
Why is this committed to the repo?
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.
Because it has to be part of the release that the action will check against. We could generate the schedule from scratch every time, but then you lose a paper trail of what was actually in it, so I personally think this solution is better.
Thanks both, I'll look at it tomorrow, so hopefully the comments will be addressed by the time you wake up again |
Co-authored-by: Nabil Freij <[email protected]> Apply suggestions from code review Co-authored-by: Nabil Freij <[email protected]> Apply suggestions from code review Co-authored-by: Brigitta Sipőcz <[email protected]>
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've addressed all comments I could. There are a few questions still open but hopefully those will be cleared up easily
spec0_action/__init__.py
Outdated
|
||
def update_dependency_table(dep_table: dict, new_versions: dict): | ||
for pkg, pkg_data in dep_table.items(): | ||
# don't do anything for pkgs that aren't in our schedule |
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.
hmm, depends on what you're imagining. it could update things that aren't in the schedule potentially but we need to know what to update it too. I'm not entirely sure what you're asking here, if you have a change you want made could you elaborate?
schedule.json
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.
Because it has to be part of the release that the action will check against. We could generate the schedule from scratch every time, but then you lose a paper trail of what was actually in it, so I personally think this solution is better.
@bsipocz I've addressed or responded to your comments. Could you take another look? |
Fixes #3
This PR makes quite a few changes but I like this approach: