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

feat: Add branch option for postUpgradeCommands #8725

Merged

Conversation

marcodejongh
Copy link
Contributor

@marcodejongh marcodejongh commented Feb 17, 2021

Changes:

Returns the old postUpgradeTask behaviour under a extra flag
Fixes #8220

Context:

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added unit tests, or
  • No new tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@marcodejongh
Copy link
Contributor Author

@sarunint Would you mind helping me on this PR?

@marcodejongh
Copy link
Contributor Author

@viceice Yeah for sure it needs to be abstracted, only reason I didn't straight away is because I wanted to make sure this change worked. I've tested it locally and it works, so I'll work on abstracting it now to get this into a mergable state later this week

lib/config/definitions.ts Outdated Show resolved Hide resolved
@marcodejongh marcodejongh force-pushed the AddBranchModeToPostUpgradeCommands branch from 44495ca to 935617d Compare March 11, 2021 05:42
@marcodejongh marcodejongh changed the title Add branch option for postUpgradeCommands feat: Add branch option for postUpgradeCommands Mar 11, 2021
@kenotron
Copy link

@marcodejongh - very much appreciate this work. An example of usage for me is to execute a bit of logic with https://microsoft.github.io/beachball/ to generate a change file for later automated semver bumping of package versions.

@marcodejongh
Copy link
Contributor Author

@rarkins @viceice I've updated the PR and pulled the post upgrade command executor to a separate file, considering how intertwined it's functionality is with the regular branch worker, I kept the tests in the branch worker itself.

@marcodejongh
Copy link
Contributor Author

@sarunint Would you mind running this branch on your codebase which I assume depends on the behavior change you made?

We'll run the newly added branch mode on our test repository

docs/usage/self-hosted-configuration.md Outdated Show resolved Hide resolved
lib/workers/branch/index.spec.ts Outdated Show resolved Hide resolved
lib/workers/branch/index.ts Outdated Show resolved Hide resolved
@sarunint
Copy link
Contributor

@sarunint Would you mind running this branch on your codebase which I assume depends on the behavior change you made?

We'll run the newly added branch mode on our test repository

Sure, I'll test it later today.

@marcodejongh marcodejongh force-pushed the AddBranchModeToPostUpgradeCommands branch from 9988fc4 to 9a6237c Compare March 22, 2021 12:45
@marcodejongh
Copy link
Contributor Author

Oops didn't intend to force push, but I didn't know how the eslint pre commit works in this repo.
All feedback has been addressed and the PR should be ready for another review

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

just small changes

if (
/* Only run post-upgrade tasks if there are changes to package files... */
!hasChangedFiles ||
getAdminConfig().trustLevel !== 'high' ||
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
getAdminConfig().trustLevel !== 'high' ||

I think if you already set allowedPostUpgradeCommands admin config we do not also require trustLevel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm are you sure? I global searched the repo and I didn't see any checks for trustLevel and allowedPostUpgradeCommands.
For now I've cleaned up the statement, no need to call getAdminConfig twice

lib/workers/pr/index.ts Show resolved Hide resolved
@viceice
Copy link
Member

viceice commented Apr 8, 2021

image
😏

lib/workers/branch/execute-post-upgrade-commands.ts Outdated Show resolved Hide resolved
lib/workers/branch/execute-post-upgrade-commands.ts Outdated Show resolved Hide resolved
lib/workers/pr/index.ts Show resolved Hide resolved
@viceice
Copy link
Member

viceice commented Apr 8, 2021

image
😉

marcodejongh and others added 2 commits April 8, 2021 17:43
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Please check and remove all casts which are not required by typescript (which you've changed).

lib/workers/branch/index.spec.ts Outdated Show resolved Hide resolved
lib/workers/branch/index.spec.ts Outdated Show resolved Hide resolved
lib/workers/branch/index.spec.ts Outdated Show resolved Hide resolved
lib/workers/branch/index.spec.ts Outdated Show resolved Hide resolved
lib/workers/branch/index.spec.ts Show resolved Hide resolved
marcodejongh and others added 3 commits April 8, 2021 19:10
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
@viceice
Copy link
Member

viceice commented Apr 8, 2021

@rarkins needs your approval

@viceice viceice enabled auto-merge (squash) April 8, 2021 09:38
@viceice
Copy link
Member

viceice commented Apr 8, 2021

@marcodejongh We can't update your branch, please use the update button, otherwise i'm not able to merge.

@marcodejongh
Copy link
Contributor Author

@viceice Hmmm I can't merge it myself either? Maybe something is setup wrong on our atlassian-forks org

@marcodejongh
Copy link
Contributor Author

Or actually I think it would be a permission on this repo that is preventing merging. In the Github UI "base branch" is the target branch

@viceice viceice merged commit 6afbcf8 into renovatebot:master Apr 9, 2021
@viceice
Copy link
Member

viceice commented Apr 9, 2021

The branch update feature does not work cross orgs, it only works for personal accounts.

An alternate is to give us write permission to your fork, so we can update your branch if required.

@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 24.109.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post upgrade tasks are now being executed for every dependency in a upgrade
8 participants