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

Feature/adds release display-name flag #1110

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

murilovarela
Copy link

@murilovarela murilovarela commented Jul 21, 2021

What is the purpose of this pull request?

This PR adds the --display-name flag to the vtex release command so the tag name is created with the app name from the manifest.

What problem is this solving?

While developing in a monorepo tags can become a bit hard to understand what tag is related to what app, so adding a folder pattern to the tag name can solve this problem. Instead of v0.10.1 we have [email protected].

Types of changes

  • Refactor (non-breaking change that only makes the code better)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Requires change to documentation, which has been updated accordingly.

Chores checklist

  • Update CHANGELOG.md

@murilovarela murilovarela self-assigned this Jul 21, 2021
@murilovarela murilovarela requested a review from a team July 21, 2021 17:38
@murilovarela murilovarela requested a review from a team as a code owner July 21, 2021 17:38
@murilovarela murilovarela changed the base branch from master to 3.x July 21, 2021 17:39
src/modules/release/index.ts Outdated Show resolved Hide resolved
@gris
Copy link

gris commented Jul 27, 2021

Let me merge/push tags/deploy this version. Since we are managing two branches, there are some tricky steps involved.

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added

- `--display-name` flag to release command

Choose a reason for hiding this comment

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

(Requesting changes just to block the PR so this comment can be seen before the merge 😅 )

@gris, I've talked to @murilovarela and we thought about adding the vendor to the tag name. This might be useful to also quickly install an app just by copying the tag name. What do you think?

Copy link

Choose a reason for hiding this comment

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

If you guys think that it is a cool idea, I'm all in!
Just out of curiosity did you guys evaluated the usage of lerna, bazel or some other monorepo tooling?
I've just noticed that having a tag referring to a specific app inside a monorepo can be misleading, specially if you are updating more than 1 app at the same time (you will have to push 2 tags pointing to the same commit).

Copy link
Author

Choose a reason for hiding this comment

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

As I see we will have two commits with the version bump change, one for each app, and the tag will be pointing to that specific commit. 🤔

Choose a reason for hiding this comment

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

Can you talk about the work you're doing in the CI bot, @murilovarela?

Copy link

@gris gris Jul 28, 2021

Choose a reason for hiding this comment

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

Hum, I see. I was just wondering if it makes more sense to have the tags point to versions of the monorepo itself, like this https://github.com/vtex/faststore/blob/master/lerna.json and https://github.com/vtex/faststore/tags

Copy link

Choose a reason for hiding this comment

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

Nevermind, I've researched a little bit more and actually saw this process implemented with tags per package in other places. I'm very curious because I will implement a monorepo for my team in the future as well :D

Copy link
Author

Choose a reason for hiding this comment

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

The CI bot doesn't use the toolbelt. It generates the tag by itself. And what we decide here, I will replicate in the ci-hub code.

Copy link
Author

Choose a reason for hiding this comment

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

So we all agree that it is a good idea to add the vendor to the tag name, correct?

Copy link

@estacioneto estacioneto Jul 28, 2021

Choose a reason for hiding this comment

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

I see, @murilovarela. I meant to share knowledge about what we're doing since @gris is curious about the monorepo initiative.

Anyway, could you add the vendor in the tag name here too? 😁

Edit: Commented before seeing the above comment 😅

Copy link

Choose a reason for hiding this comment

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

All good, it makes sense to have the vendor for me.

@murilovarela murilovarela reopened this Nov 5, 2021
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.

3 participants