Conversation
mdellweg
force-pushed
the
cleanup
branch
3 times, most recently
from
73f4ef9 to
c13c1ea
Compare
pedro-psb
left a comment
pedro-psb
left a comment
There was a problem hiding this comment.
There are lot of changes in the before_install / install scripts. From reading it that looks fine, but I would like to see a test PR.
Btw, looking much better, thanks!
| echo "No formatting change needed" | ||
| fi | ||
| fi | ||
|
|
There was a problem hiding this comment.
The convenience about this (idea) was having a separate commit for formatting, because the CI update PRs squash all changes from plugin-templates in a single PR. But In the end I think this wasn't even working properly, since we have a format call on the plugin-template apply itself.
There was a problem hiding this comment.
My idea is to not even make the first commit before the format linter accepts it. If that reformats a bigger part of the codebase, well that's fine for me too.
There was a problem hiding this comment.
This isn't even yet renderd by default. But I think we want to go the Makefile route and one needs a place to start from.
| if: "steps.create_pr_changelog.outputs.pull-request-number" | ||
| env: | ||
| GH_TOKEN: "{{ '${{ secrets.RELEASE_TOKEN }}' }}" | ||
| continue-on-error: true |
There was a problem hiding this comment.
We've seen some flaky bugs with changes aggregation but 99% it just works.
And we've reviewed the fragment on the PRs already. I'm fine with that.
There was a problem hiding this comment.
I think this ignore error only applies to the attempt to mark the PR automerge.
|
|
||
|
|
||
| def check_pyproject_dependencies(repo, from_commit, to_commit): | ||
| def check_pyproject_dependencies(repo: Repo, from_commit:str, to_commit:str) -> list[str]: |
There was a problem hiding this comment.
Why black didn't complain about that from_commit:str in the CI here?
If this file is applied at the target repository and we run black against it, it will fail.
There was a problem hiding this comment.
I guess, since reformatting is part of the templating there is nothing to complain about afterwards.
| return 1 | ||
|
|
||
| branches = [branch for branch in available_branches if branch in branches] | ||
| branches.reverse() |
There was a problem hiding this comment.
Reverse doesn't sort the branches. Maybe .sort(reverse=True)?
There was a problem hiding this comment.
They are already sorted at this point.
|
|
||
| {% include 'header.j2' %} | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
Can you put a headline comment here with what is the responsability of this script.
Similar to how you've added in templates/github/.github/workflows/scripts/before_script.sh.j2
| cmd_prefix bash -c "usermod -a -G wheel pulp" | ||
| echo | ||
| echo "# pip list outside the container" | ||
|
|
There was a problem hiding this comment.
I don't see the pip list foor outside the container.
There was a problem hiding this comment.
Hmm, it's on the install.sh. I guess before_script is after install, right? We can move it here, then.
There was a problem hiding this comment.
It would be nice to have the github grouping thing, if you agree.
echo ::group::GROUP_NAME
...
echo ::endgroup::
There was a problem hiding this comment.
I'm trying to regroup all the stuff on the jobs.steps level now. This extra folding sadly, is implemented by adding noise to the output.
But I'll try to remove the -v from the bash settings. Since this file is really only for dumping information (now) we don't want to see all the noise from reading the script.
Let's look at that and decide from there.
There was a problem hiding this comment.
Ok. Its fine not having the github group thing too. Having it all on one script is already quite helpful.
|
I tried the changes here: pulp/pulp_gem#420 |
2 participants
No description provided.