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(config): improve monorepo names #10243

Merged
merged 31 commits into from
Jul 30, 2021

Conversation

HonkingGoose
Copy link
Collaborator

@HonkingGoose HonkingGoose commented May 31, 2021

Changes:

  • General cleanup of monorepo names

Context:

Found a lot of inconsistencies, so I'm labeling this a breaking change. 😄

Some repos have moved, some have deprecation notices in them, one is even outright gone.

Closes #10210.

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

@HonkingGoose HonkingGoose added the breaking Breaking change, requires major version bump label May 31, 2021
@HonkingGoose
Copy link
Collaborator Author

Unsure if I need to add the ! character in the PR title or not.
Conventional Commit style seems to dictate that you use a ! after type/scope to denote breaking changes.

Feel free to remove the ! character from the PR title if this will lead to problems. 😉

@rarkins
Copy link
Collaborator

rarkins commented Jun 1, 2021

Unsure if I need to add the ! character in the PR title or not.
Conventional Commit style seems to dictate that you use a ! after type/scope to denote breaking changes.

Feel free to remove the ! character from the PR title if this will lead to problems. 😉

Can you check what semantic-release supports?

@HonkingGoose

This comment has been minimized.

@HonkingGoose
Copy link
Collaborator Author

HonkingGoose commented Jun 1, 2021

Quote from semantic-release/semantic-release#1796 (comment):

based on this log output, it looks like you are including the BREAKING CHANGE: in the subject of the commit. the default convention used by semantic-release expects this to be in the commit body, unlike feat, fix, etc because any commit type can include a breaking change

Also there's this from semantic-release README (https://github.com/semantic-release/semantic-release#commit-message-format):

Here is an example of the release type that will be done based on a commit messages:

Commit message Release type
fix(pencil): stop graphite breaking when too much pressure applied Patch Release
feat(pencil): add 'graphiteWidth' option Minor Feature Release
perf(pencil): remove graphiteWidth option

BREAKING CHANGE: The graphiteWidth option has been removed.
The default graphite width of 10mm is always used for performance reasons.
Major Breaking Release

I think the only thing semantic-release supports is putting BREAKING CHANGE: in the commit body.

@HonkingGoose HonkingGoose changed the title feat(config)!: improve monorepo names feat(config): improve monorepo names Jun 1, 2021
@travi
Copy link
Contributor

travi commented Jun 3, 2021

I think the only thing semantic-release supports is putting BREAKING CHANGE: in the commit body.

this is true because we still use the angular convention by default today. we intend to switch that default to conventional commits in the future, and that change can be tracked here. until then, you could change your convention preset, if you'd like.

@HonkingGoose
Copy link
Collaborator Author

It seems we're already using the conventional commit preset from @semantic-release/commit-analyzer:

"preset": "conventionalcommits",

Link to file on current main: https://github.com/renovatebot/renovate/blob/main/.releaserc

What do you think @travi? Would it be safe to use a PR title like this: feat(config)!: improve monorepo names so with the ! to signify breaking change?

I'll keep things as they are until we know for sure how we've configured semantic-release on our end. 😄

@travi
Copy link
Contributor

travi commented Jun 3, 2021

Would it be safe to use a PR title like this: feat(config)!: improve monorepo names so with the ! to signify breaking change?

assuming you are mentioning the PR title because the PR will be squashed (and since the PR title is used as the default commit message when squashing), i would expect that to result in being treated as a breaking change, as you expect. it is important to keep in mind that semantic-release does not care about the actual PR, but only the resulting commits.

@HonkingGoose
Copy link
Collaborator Author

assuming you are mentioning the PR title because the PR will be squashed

All our PRs get squashed into one commit, with commits from the PR branch going in the commit body, and the PR title as commit message.

i would expect that to result in being treated as a breaking change, as you expect

Cool!

it is important to keep in mind that semantic-release does not care about the actual PR, but only the resulting commits.

Will do!

@HonkingGoose
Copy link
Collaborator Author

@rarkins I've left some comments in the code, can you maybe review the PR and tell me what you think?

@HonkingGoose
Copy link
Collaborator Author

Summary: I'm bad at dealing with merge conflicts, and need a way to avoid them on this PR. 😉


I'm having trouble fixing the merge conflict(s) on this PR every time I merge the current main branch.

The basic problem:

  1. HonkingGoose creates breaking change in the monorepo config file
  2. Renovate maintainers merge in new lines in monorepo config file to the main branch
  3. HonkingGoose updates this PR to be current with main: merge conflict ☠️
  4. Manually fix merge conflict, trying not to mess up the lines that are good as-is

I'm bad at dealing with merge conflicts, and rather avoid them completely. 🙈
Because my change is a breaking one, it's expected that things break regularly... 😄

I think the root cause is that I'm trying to keep up-to-date with a branch that's moving under me. Which will always end badly... 😄

I think we have the following options to get out of this mess:

  • Abandon PR and try again once we're gearing up for a breaking release anyways and are willing to hold off adding new monorepo lines, so I can focus on cleaning up the existing lines
  • Retry this PR on current main, with the promise from Renovate maintainers to stop merging new monorepo lines (probably a non-starter)
  • Renovate team doesn't really care about this: close PR, and stop working on this problem

Or maybe one of you has a great idea? I'm open to suggestions! 😄

lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/monorepo.ts Outdated Show resolved Hide resolved
HonkingGoose and others added 2 commits July 28, 2021 09:57
Co-authored-by: Rhys Arkins <rhys@arkins.net>
Co-authored-by: Rhys Arkins <rhys@arkins.net>
@HonkingGoose
Copy link
Collaborator Author

@rarkins maybe this should go in the v26 roll-up PR/branch as well? Let me know if I need to do anything... 😉

@rarkins rarkins changed the base branch from main to v26 July 29, 2021 20:01
@rarkins rarkins marked this pull request as ready for review July 29, 2021 20:01
@rarkins rarkins requested a review from JamieMagee as a code owner July 29, 2021 20:01
@rarkins
Copy link
Collaborator

rarkins commented Jul 29, 2021

@HonkingGoose I'll merge it into v26 if you're happy with it

@rarkins rarkins merged commit 2a89eee into renovatebot:v26 Jul 30, 2021
@HonkingGoose HonkingGoose deleted the breaking/improve-monorepo-names branch July 30, 2021 10:05
rarkins pushed a commit that referenced this pull request Aug 2, 2021
rarkins pushed a commit that referenced this pull request Aug 9, 2021
rarkins pushed a commit that referenced this pull request Aug 13, 2021
rarkins pushed a commit that referenced this pull request Aug 13, 2021
rarkins pushed a commit that referenced this pull request Aug 15, 2021
rarkins pushed a commit that referenced this pull request Aug 16, 2021
rarkins added a commit that referenced this pull request Aug 17, 2021
rarkins added a commit that referenced this pull request Aug 17, 2021
@tomkerkhove
Copy link
Contributor

Not sure if there is a pending release for this, but it's breaking updates in v26.1.2

@HonkingGoose
Copy link
Collaborator Author

@tomkerkhove we put these changes into a v26 release as changing the monorepo names change the branch names as well, which can cause breakage.

Can you provide more details on how things are going wrong in v26.1.2?

Pinging @rarkins to listen to this conversation as well.

@rarkins
Copy link
Collaborator

rarkins commented Aug 18, 2021

The app is not yet updated to v26. @tomkerkhove can you provide more details?

@tomkerkhove
Copy link
Contributor

We are getting the following:

INFO: Throwing preset error (repository=iot-platform/exedra-asset-management)
       "validationError": "Cannot find preset's package (group:SwashbuckleMonorepo). Note: this is a *nested* preset so please contact the preset author if you are unable to fix it yourself."

Where we had to change this in our config:

    "config:base",
    ":gitSignOff",
    "group:azure azure-libraries-for-netMonorepo",
    "group:azure azure-sdk-for-netMonorepo",
    "group:azure azure-storage-netMonorepo",
-    "group:swashbuckle-aspnetcoreMonorepo"
+    "group:SwashbuckleMonorepo"
  ]

Same for ASP.NET Extensions:

 INFO: Throwing preset error (repository=iot-platform/exedra-asset-management)
       "validationError": "Cannot find preset's package (monorepo:aspnet Extensions). Note: this is a *nested* preset so please contact the preset author if you are unable to fix it yourself."
 INFO: Repository has invalid config (repository=iot-platform/exedra-asset-management)
       "error": {
         "validationError": "Cannot find preset's package (monorepo:aspnet Extensions). Note: this is a *nested* preset so please contact the preset author if you are unable to fix it yourself.",
         "message": "config-validation",
         "stack": "Error: config-validation\n    at resolveConfigPresets (/home/vsts/.npm/_npx/1776/lib/node_modules/renovate/dist/config/presets/index.js:265:35)\n    at processTicksAndRejections (internal/process/task_queues.js:95:5)\n    at async resolveConfigPresets (/home/vsts/.npm/_npx/1776/lib/node_modules/renovate/dist/config/presets/index.js:311:38)\n    at async Object.resolveConfigPresets (/home/vsts/.npm/_npx/1776/lib/node_modules/renovate/dist/config/presets/index.js:289:38)\n    at async Object.mergeRenovateConfig (/home/vsts/.npm/_npx/1776/lib/node_modules/renovate/dist/workers/repository/init/merge.js:210:50)\n    at async Object.getRepoConfig (/home/vsts/.npm/_npx/1776/lib/node_modules/renovate/dist/workers/repository/init/config.js:12:14)\n    at async Object.initRepo (/home/vsts/.npm/_npx/1776/lib/node_modules/renovate/dist/workers/repository/init/index.js:26:14)\n    at async Object.renovateRepository (/home/vsts/.npm/_npx/1776/lib/node_modules/renovate/dist/workers/repository/index.js:64:18)\n    at async Object.start (/home/vsts/.npm/_npx/1776/lib/node_modules/renovate/dist/workers/global/index.js:109:13)\n    at async /home/vsts/.npm/_npx/1776/lib/node_modules/renovate/dist/renovate.js:33:24"
       }

Note that these are written as INFO so our pipeline passes, while Renovate couldn't do anything

This is OK btw, just want to make sure you are aware of it.

(this is a self-hosted scenario)

@rarkins
Copy link
Collaborator

rarkins commented Aug 18, 2021

We put it in a major release intentionally, although I guess we failed to declare it as breaking:

image

@viceice @HonkingGoose I think we should add migration code to migrate all the old names to new names automatically. Otherwise we might see a lot of this suddenly in the app once it goes live

@tomkerkhove
Copy link
Contributor

That would be nice to have indeed, thank you for that!

But you are correct - We just use the latest version so this is OK but just wanted to let you know in case you were not aware of it.

Maybe the error can have a link to the notes or so?

@rarkins
Copy link
Collaborator

rarkins commented Aug 18, 2021

Definitely thank you for reporting it as I wasn't aware yet. I think best we do automigration internally so that others like you don't even need to think about it.

@tomkerkhove
Copy link
Contributor

Thanks a ton!

@viceice
Copy link
Member

viceice commented Aug 18, 2021

Yes. 🤦‍♂️

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking Breaking change, requires major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use better monorepo names in monorepo.ts
5 participants