-
Notifications
You must be signed in to change notification settings - Fork 28
Github action to update FLS every 24 hours #191
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
✅ Deploy Preview for scrc-coding-guidelines ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Given #195 has been merged, maybe it's a good idea to revert the second commit? |
|
Hey @plaindocs, following up here to see if you'd be able to look into the feedback. |
|
@plaindocs I'm really looking forward to this! ^^ I just ran again into the need to update the FLS because it was blocking #180 and #181 today :') |
|
I'm going to make time to look at this again, soz for the delay. |
|
@felix91gr @PLeVasseur ok, I've got a stupid bash script which lets us decide whether to update automatically. Should be easy to connect that to GHA input/output to make the PR conditional on that. But I ran out of time today before heading off for two weeks. Happy for someone to pick it up and finish it, or I can look when I'm back. |
| dest = root / "build" | ||
|
|
||
| args = ["-b", builder, "-d", dest / "doctrees"] | ||
| args = ["-b", builder, "-d", dest / "doctrees", "-wtest.txt"] |
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.
This needs moving out into a flagged parameter so it doesn't happen with every build.
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.
Under what circumstances does it need to be included?
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'm using that text file to parse out whether we can open the PR to update the build or not, so it's only really needed for this job.
But there isn't a trivially built in way to pass something into that custom build script. I could probably do something with the job name 🤔
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.
This needs moving out into a flagged parameter so it doesn't happen with every build.
What would be the consequence if it happened on every build? @plaindocs
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.
To summarize what we discussed earlier:
- @plaindocs will update
builder/build_cli.pyto expose the required parameter - Use the exposed parameter and run
builder/build_cli.pywhere appropriate
Did I get that right @plaindocs, @felix91gr?
|
I've not looked into the Python version build error. |
|
OK this maybe sorta works at this point. Hard to test, although I suppose I could manually revert the FLS code, so it has to update it. @felix91gr any idea on how to easily move this parameter out so it's not happening on every run? |
I did look at Dependabot, but I don't think it quite works for this scenario.