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

Dependecy update auto merge after passing tests #3741

Closed

Conversation

Ali-Razmjoo
Copy link
Sponsor

  • I have read and understand the pull request rules.

Description

Following the discussion Automatic Dependencies Updates, I have written two new configuration files. By using this, you don't have to click merge every time or execute the npm update; either dependency updates made by dependabot will automatically merge after passing CI tests.

The reason this is more beneficial is that we can receive dependency updates, including security updates, without waiting for feature pull requests to finish and it won't block continued development or add more hassles and manual work.

Files descriptions:

  1. .github/dependabot.yml: Configures Dependabot to update npm packages, Docker, and GitHub Actions daily.
  2. .github/workflows/dependabot-auto-merge.yml: Sets up an auto-merge GitHub Actions workflow for Dependabot PRs if they pass the tests.

Type of change

Please delete any options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Automation
  • Security

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

  • Not applicable.

@Zaid-maker
Copy link
Contributor

Can we make Dependabot only make patch and minor dependencies updates? Because Major versions can break the app

@harryzcy
Copy link
Contributor

harryzcy commented Sep 15, 2023

Can we make Dependabot only make patch and minor dependencies updates? Because Major versions can break the app

I think dependabot should still create the PRs for major versions, and just don't auto merge them.

Or alternatively, I think Renovate might be a better choice, and it takes care of auto merge out-of-box.

@Ali-Razmjoo
Copy link
Sponsor Author

  • Yes we can definitely do that; but before that are all functions are have CI tests? If yes that wouldn't be necessary since it will not pass the tests so it won’t merge. (Will work on it tomorrow or Sunday)
  • in case tests are not complete we can maybe tests ui functionalities with a webdriver to check if everything works as expected; and send a new pr for that. I know how to develop that but I am unsure how to structure them in the best way; I am not really a software engineer. Give me some high-level structure and I will send the new PR. I was thinking just make a flow to
    • setup a user
    • Login
    • test functionalities one by one (e.g. configuring settings, setup monitors, reset password, etc).

Does it sound good?

@harryzcy
Copy link
Contributor

harryzcy commented Sep 15, 2023

are all functions are have CI tests?

No, there're only very few tests, so automerge may not work so far

@Zaid-maker
Copy link
Contributor

  • Yes we can definitely do that; but before that are all functions are have CI tests? If yes that wouldn't be necessary since it will not pass the tests so it won’t merge. (Will work on it tomorrow or Sunday)
  • in case tests are not complete we can maybe tests ui functionalities with a webdriver to check if everything works as expected; and send a new pr for that. I know how to develop that but I am unsure how to structure them in the best way; I am not really a software engineer. Give me some high-level structure and I will send the new PR. I was thinking just make a flow to
    • setup a user
    • Login
    • test functionalities one by one (e.g. configuring settings, setup monitors, reset password, etc).

Does it sound good?

Yes this sounds great

@Ali-Razmjoo
Copy link
Sponsor Author

I have added the condition that if update-type doesn't contain the word major; it will be automatically merged, otherwise it keeps the PR open until someone manually merges it.

@Zaid-maker
Copy link
Contributor

I have added the condition that if update-type doesn't contain the word major; it will be automatically merged, otherwise it keeps the PR open until someone manually merges it.

Thank you so much! I guess this should be merged

@Zaid-maker
Copy link
Contributor

@louislam can u merge this PR please

@chakflying
Copy link
Collaborator

chakflying commented Sep 22, 2023

I think improvements need to be made on unit & integration tests before any attempt at auto-merge. The fact is that there is currently very little test coverage for a project that has grown much in complexity.

@Ali-Razmjoo
Copy link
Sponsor Author

@chakflying, what is the current process for going live? Is manual testing done on all features every time?

@chakflying
Copy link
Collaborator

Can't speak on behalf of @louislam as he does the releasing. I only test features per-PR as they are merged.

@louislam
Copy link
Owner

louislam commented Oct 5, 2023

At this moment, I prefer running npm update before release which I feel more comfortable.

If dependabot auto merges pull requests, I don't know, but it doesn't seem to be safe for me. As @chakflying said, the code coverage in this project is pretty much no coverage.

If dependabot keeps creating pull requests here, I feel like it increases my workload to review them, especially some packages literally update everyday such as those aws packages.

However, if there are any other bots that could create a single pull request. I am OK with it. Like our Weblate translation bot: #3659

@Zaid-maker
Copy link
Contributor

Zaid-maker commented Oct 9, 2023

If dependabot auto merges pull requests, I don't know, but it doesn't seem to be safe for me. As @chakflying said, the code coverage in this project is pretty much no coverage.

I don't get what u mean by if Dependabot create PR's not safe for you? Auto merge can decrease your workload and maybe u assign someone to review the PR's and when they approve it Dependabot merge the PR.

Also how we are gonna able to increase code coverage?

@louislam

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Feb 3, 2024

@Ali-Razmjoo
Thank you for this contribution, but I am going to close this PR.

The rationale is, that currently updating the dependencies before every release works.
As for the automerging, share louis and nelson's strong reservations given how untested most of the codebase is.
We likely would not notice any defects until reports by users start coming in if we don't read through the changelogs as we currently do. Doing this automatically is given the tests we have just not a good idea.

Personal opinion:
If a tool were to be included, it would have to be a less noisy one like renovate, with the config set to group all upgrades into one PR.
The relevant configuration for the renovate.json should be

{
  "extends": ["config:recommended", "group:all"]
}

We can reopen this PR

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.

None yet

6 participants