-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Git Only feature #1497
base: main
Are you sure you want to change the base?
Git Only feature #1497
Conversation
Here is the summary of changes: Background info on repo (to double check my understanding):
Approach:
That's it! P.S. even for the case there are multiple packages, there shouldn't be a problem. Because, it is already working with multiple packages without the The changes I propose will also fetch the workspace cargo.toml file if that's the case, so it should be good to go imo. @MarcoIeni please let me know if I missed anything. Also, we can discuss how Remaining work:
|
Yes, it "releases" the current state of the repo, i.e. unpublished crates, running The rest of the background info looks correct 👍 The approach looks correct, too! I will review this PR this soon!
|
I added the new line to the schema in #1500 so that your tests don't fail 👍 |
Draft implementation is complete. I'll take a look at the tests and try to write the test you mentioned as the next step. I might be busier than usual this week, but I'm definitely committed to finish this feature. If you @MarcoIeni or someone else wants to chip in to make the process faster, feel free to do so! |
repository.repo.checkout(&tag)?; | ||
Ok(input.local_manifest.clone()) | ||
} | ||
None => return Err(anyhow!("there is no previous tag to compare with!")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case, instead of throwing an error, we should release v0.1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to ask that. Didn't inspect the code for that path (where registry package is not found). So, release-plz
already does not require previous version to exist, and can release the first version (a.k.a v0.1.0) when registry package is not found, am I right?
/// Returns the latest tag of the repository in the form of `vX.X.X` | ||
/// * `None` if there are no version tags found matching the regex, | ||
/// * `Tag` if at least one tag is matching the regex | ||
pub fn get_repo_versions(repo: &Repo) -> Option<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function name is plural, but you are returning a string.
Also, workspaces tags are in the form of {package}-v{version}
. E.g. https://github.com/MarcoIeni/release-plz/tags
Does the implementation of this PR recognize the right tag for the right package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function name is plural, but you are returning a string.
Will fix the name, thanks for the catch!
Also, workspaces tags are in the form of {package}-v{version}.
regex can detect package
and v{version}
separately via subgroups and we can do anything we want with them. I'll update the function so that it will return package-v{version}
instead of v{version}
Does the implementation of this PR recognize the right tag for the right package?
Ok, here is the confusion on my end about this topic.
- Let's say we want to release package
a-v1.0.1
. - And assume we have these two following releases in the repo:
a-v1.0.0
-> released at last yearb-v1.0.0
-> released just yesterday (more recent compared toa
)
Even though we will be updating the package a
, should we use the tag a-v1.0.0
as the comparison for the workspace repo, or should we checkout to b-v1.0.0
? I'm honestly confused about that, and I believe you can make a better decision than me @MarcoIeni on this. Although it's counter-intuitive, I argue b-v1.0.0
is a better choice. Due to:
- in tag
b-v1.0.0
, the version of packagea
is alsov1.0.0
, so nothing changed abouta
. - for the changelog generation, it might be simpler to compare the current version against the latest release to eliminate duplications across release notes.
However, I don't know how differences are calculated and how changelog is generated (did not spend that much time surfing the repo). So my assumptions may be invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use the tag a-v1.0.0 as the comparison for the workspace repo, or should we checkout to b-v1.0.0?
I would use tag a-v1.0.0
if we are comparing a
, and tag bv-1.0.0
if we are comparing b
.
This mirrors better the current behavior of release-plz, when we compare each package with the version published in crates.io
in tag b-v1.0.0, the version of package a is also v1.0.0, so nothing changed about a.
I'm not sure about this, maybe people released b
, but they didn't release a
yet. But this new tag contains unreleased changes of a
.
The difference is calculated by analyzing every commit that touches the files of a package. Each commit is added to the changelog.
So I don't see issues of duplication across release notes. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe people released b, but they didn't release a yet.
Makes sense! I'll update the logic according to this, so it will detect the latest version for each package
For the scope of this PR ( I could use some clarifications on the behavior of this function: First point:
If these assumptions above are true, I think we can leave the Second point:
If above understanding is correct, then I believe the code should be changed as the following:
No change should be required from what I can tell. When you have the time, I'd appreciate a sanity check on these @MarcoIeni |
First pointYour assumptions seem correct to me. However, I'm not sure about the following:
If registry_manifest is None in the Second pointEverything looks good here. More specifically, you are right that the |
Ok good that we are discussing this, because maybe I misunderstood something. First point
You are right, and I think we already got it covered. But I probably did not clearly explain my ideas. My understanding was:
This means, I can write the below logic for
Second pointThis is where I'm still confused.
Unfortunately, this doesn't make sense to me. I'm probably missing some details for the multi-package scenario. When you have the time, if you can see my comment, maybe we can continue this discussion there. |
Yes, if your project doesn't have naming conflicts with projects present in crates.io! For example, if your project is called I replied to that comment (sorry, I missed it!). Happy to provide more help if needed 👍 |
Hi @MarcoIeni, was a busy week so I'm taking a look just right now. Btw, thanks a lot for your responsiveness, great communication! The code changes are trivial, however the biggest problem is not changing the code for this PR. Whilst I was changing the code, I found the root of my confusion. I'll explain it below: this is
|
Thank you for spending time on this issue 😄
Yes, because the manifest can be a "workspace manifest". For example, take a look at the Cargo.toml in the root of this repository.
Yes 👍
Yes 👍
No problem, this issue is very difficult, it's normal that you ask doubts!
I thought about the following algorithm, but maybe I'm missing something:
|
Great! This resolves my biggest confusion. I was and am planning to provide a single
But now I realized you asked if the function is able to retrieve latest tags for all packages NOT for the This whole discussion is basically about me being lazy by not looking at the code and trying to understand how this project works for the multiple packages case 😅 Coding the solution is pretty easy after understanding all these. I'm on vacation for the last week, hence the late reply. I'll take a look at the codebase once my vacation is over. |
Great, enjoy your vacation :D |
|
Let's have a call, yes. We can even work on this together with pair programming 👍 |
Summary of our callget_registry_packagesFor the git-only feature, IN THEORY we can't rely on the For example, one could:
Now when we run The solution to this is to To avoid performance issues, we could run diff algorithmIn the diff algorithm, we need to |
Aims to fix #1144.
I'll edit the PR explanation once the discussions below are finalized.