Skip to content
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

Introduce npmCheckDay parameter to update-deps workflow #28

Closed
wants to merge 18 commits into from

Conversation

Dumluregn
Copy link
Contributor

For now I've added a config parameter: npmCheckDay for update-deps.yml workflow. It allows to choose one day in the month when npm-check (non-semver deps bump) should be run instead of the usual npm update (semver deps bump).

README update about update-deps.yml workflow should be done after the PR passes review to avoid correcting it multiple times if something changes in the meantime.

Closes #10
Closes #21
Closes #26
Closes #27

@f1ames f1ames self-requested a review June 2, 2021 14:11
@f1ames f1ames self-assigned this Jun 2, 2021
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Generally, looks good 👍 But I have one concern (see below).

README update about update-deps.yml workflow should be done after the PR passes review to avoid correcting it multiple times if something changes in the meantime.

👍

I wonder about the approach here, having a specific day of the month means we are not able to do major deps update less frequently than once per month (well, it is exactly once a month or not at all). I was thinking about it, but anything else (which seems reasonable) would require saving a state (e.g. major update every X runs).

So the ideal scenario would be having two separate runs or workflows for each update type. For example:

  • Regular update runs every two weeks 0 5 1,15 * *.
  • Major update runs whenever we configure it using valid cron schedule.

But this is problematic because:

  • This will require having two separate workflows here so the cron can be configured separately and it can be easily used in the target repo (a lot of code duplication unless we decide to extract common parts which may be complex).
  • Or coming up with some smart mechanism for workflows duplication in the target repo.
  • Or creating smart job matrix which will know when to run which update (not sure how it could work).
  • Or having update-deps do regular update by default and major only when it is triggered with a specific option. And then have separate workflow which will trigger it (like this or this).

☝️ The last one seems to be most promising IMHO, but it's still a bit complex. Maybe sticking to simple solution which we have now would be enough 🤔 WDYT @Dumluregn?

Comment on lines +124 to +126
* `GH_WORKFLOWS_TOKEN` (same as `AUTH_KEY` variable);
* `GH_BOT_EMAIL` (most likely you );
* `GH_BOT_USERNAME`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it depends - if you create a repo in ckeditor org (which below sample env file contents suggests), our standard GH_BOT_* values should work here? Or not 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR they didn't work and you need to provide your own... ;( They seem to be active if you take a look into repo settings, but they are not working with the script... If you override them with the new one - it works fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

☝️ The last one seems to be most promising IMHO, but it's still a bit complex. Maybe sticking to simple solution which we have now would be enough 🤔

TBH, I think the solution with a separate workflow that dispatches another workflow seems to be most valid for me. We can freely manipulate the schedule with cron. If we decide that the current approach (once a month) is no longer valid we'll be forced to adjust a lot of code. We do the effort now and have a more elastic solution.

Comment on lines 14 to 23
name: 'update-deps on up-to-date branch',
workflow: 'update-deps.yml',
branch: 'master',
config: {
'targetBranch': 'master'
},
fileList: [ {
src: 'deps-package-up-to-date.json',
dest: 'package.json'
} ]
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an issue with this test that after a while the packages (see https://github.com/ckeditor/ckeditor4-workflows-common/pull/28/files?file-filters%5B%5D=.js&file-filters%5B%5D=.json&file-filters%5B%5D=.yml#diff-aec01d3969cc571603b309d74638b18fd1eff39fc547aa979fc6b27c9dc0e86cR11) may not be up to date 🤔 So we probably need to have previous major latest minor like ^2.6.12 for core-js, since ^3.13.0 will cause an update already (checked with https://semver.npmjs.com/):

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, even if we use ^2.6.12, npm-check will update it anyway 🤔 So, two workflows for npm-check and npm update looks even more valid.

@sculpt0r
Copy link
Contributor

sculpt0r commented Aug 3, 2021

Hi @f1ames - PR is ready for another review.

Changes:

  • new workflow with tests was created: bump-deps. It uses outer workflow mentioned by you earlier. This workflow simply reads the config and overrides/adds the bump property to the config. Config is passed to the next workflow (update-deps).
  • Removed day checking from the update-deps workflow and replaced it with verifying dump property in config
  • the bump-deps workflow is set to be run once a month via cron
  • lower version of package in test input

@sculpt0r sculpt0r removed their assignment Aug 3, 2021
@sculpt0r sculpt0r requested a review from f1ames August 3, 2021 09:57
@sculpt0r
Copy link
Contributor

sculpt0r commented Aug 5, 2021

@jacekbogdanski jacekbogdanski requested review from jacekbogdanski and removed request for f1ames August 24, 2021 10:43
@jacekbogdanski
Copy link
Member

@sculpt0r dependabot is still not allowing grouping updates dependabot/dependabot-core#1190

But I see that there is some other tool which may help us if we would like to use existing solution: https://github.com/renovatebot/renovate

@@ -0,0 +1,40 @@
name: Bump dependencies
Copy link
Member

Choose a reason for hiding this comment

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

We have 2 workflows now - bump deps and update deps. There is no way to tell what's the difference between them by the name. Maybe we should be more specific in this regard?

Also, it would be good to document both workflows using yaml comments, so some generic description + what's happening in the code.

workflows/update-deps.yml Show resolved Hide resolved
@sculpt0r sculpt0r self-assigned this Sep 16, 2021
@sculpt0r
Copy link
Contributor

Rebase on newest master

@sculpt0r
Copy link
Contributor

@jacekbogdanski - ready for another review

  • I kept separate workflow files (as we discussed together),
  • renamed 'update deps' workflows to clarify their purposes. It will require some clean-up in other projects - since you discover that our sync workflow may not remove deleted files. However, it will be easier to make such clean-up now, than constantly looking into code to find out which workflow respects semversioning,
  • updated README with an explanation about two workflows connectivity,
  • switched to a less precise Node version in .nvmrc file.

@sculpt0r sculpt0r removed their assignment Sep 27, 2021
@jacekbogdanski jacekbogdanski changed the title Introduce npmCheckDay parameter to update-deps workflow. Introduce npmCheckDay parameter to update-deps workflow Sep 28, 2021
@jacekbogdanski
Copy link
Member

@sculpt0r could you freeze this ticket for now? It seems like the issue is not that problematic as we thought and we can wait with implementing it.

@sculpt0r sculpt0r added the status:pending More information is needed to proceed with the issue. label Nov 2, 2021
@sculpt0r
Copy link
Contributor

sculpt0r commented Nov 2, 2021

❄️ Let's wait with this issue - until we decide if the current bot-updates flow is really unpleasant.

@sculpt0r
Copy link
Contributor

sculpt0r commented May 6, 2022

Since we decide to handle dependencies on our own and it is a long time since we do not back to this PR - I'm closing it.

@sculpt0r sculpt0r closed this May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:pending More information is needed to proceed with the issue.
Projects
None yet
4 participants