Skip to content

feat: Add auto-pr cron job #1314

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

Closed
wants to merge 2 commits into from
Closed

feat: Add auto-pr cron job #1314

wants to merge 2 commits into from

Conversation

nschonni
Copy link
Member

No description provided.


on:
schedule:
- cron: 0 0 * * *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment about what this means so I don't have to use https://crontab.guru/?

Co-authored-by: mary marchini <[email protected]>
@SimenB SimenB requested a review from a team August 16, 2020 10:19

on:
schedule:
- cron: 0 0 * * *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- cron: 0 0 * * *
- cron: 0 * * * *

Every hour instead of every day? Otherwise I assume people will be posting issues wondering when the next release will be available if it's potentially 23 hours delayed.

Could run it event more often of course, not like it takes a lot of resources/time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nschonni I still think this makes sense, fwiw

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think every hour is better 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or even more often - any issue running this every 10 minutes?

@SimenB
Copy link
Member

SimenB commented Oct 6, 2020

@nschonni land this? It's been 1.5 months without objections from @nodejs/docker folks or others

Comment on lines +11 to +14
version:
- 10
- 12
- 14
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will create 3 PRs if there are updates for all release lines, right? Should we instead just do ./update.sh without passing a version and use some node script to construct the PR title and body?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

./update.sh is broken atm and just runs against all versions even when you pass one as an argument.
Also, this will create PRs for yarn updates too even when there isn't a node update, since it doesn't have the -s flag.

I am interested in fixing update.sh or even perhaps porting it to a node script, if that is desired. It could be made more useful like returning what node versions it updated, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

migrating to a js script makes sense to me.

As a temporary workaround, the script could run with -s first, and only if there is a diff, run again without the -s. But yeah, might make more sense to port it to a js script so it's a bit easier to work with more advanced logic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should happen if there is a keys update (like there was a few days ago). Should any images be updated if there are no node updates?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, it should only create a PR if the Node version line changes. Key changes should only go in with a version bump

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll add some logic to grab the keys in the existing files to replay them on the templates, unless the node version changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably just try looking at the git diff result

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should only create a PR if the Node version line changes

There might be bug fixes and such, so I think it's better to say that

  1. yarn version change
  2. key changes

only triggers an update if the node version changes - all other changes than those two should always trigger

Base automatically changed from master to main March 15, 2021 16:23
@nschonni nschonni closed this May 1, 2022
@nschonni nschonni deleted the auto-pr branch May 1, 2022 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants