fix: Not all commits beeing evalutated, added all option to gitlog #48

Forestsoft-de · 2022-10-02T23:48:36Z

In my case only merge commits are evalulated. I dont know why but get-next-version always decide that there are only chore commits.

After adding All: true it works.

BR
Sebastian

goloroden

Hey @Forestsoft-de 👋

First of all: Thank you very much for your PRs, we appreciate this a lot 😊

I've decided to first focus on this one, as this is the easier one, and the other one needs a little bit more of discussion. I hope this is fine for you.

I've reviewed your changes, and left a few comments. Could you please take care of them?

In addition it would be great if we could have a test that reproduces the faulty behavior. Can you please add one?

Have a nice day,

Golo

goloroden · 2022-10-03T07:36:07Z

.gitignore

@@ -6,3 +6,4 @@ coverage/

 # OS generated files and directories
 .DS_Store
+get-next-version


This shouldn't be needed, if the build is run using the Makefile, with the command make build. Can you please revert this change?

goloroden · 2022-10-03T07:39:28Z

git/commits.go

@@ -29,7 +29,7 @@ func GetConventionalCommitTypesSinceLastRelease(repository *git.Repository) (Con
 }
 return ConventionalCommmitTypesResult{}, err
 }
- commitIterator, err := repository.Log(&git.LogOptions{From: head.Hash()})
+ commitIterator, err := repository.Log(&git.LogOptions{From: head.Hash(), All: true, Order: git.LogOrderCommitterTime})


According to the documentation of the go-git module using From and Order at the same time is incompatible. I.e., if Order is being used, From is ignored. So we should remove one of these. If it's actually needed, we should remove From. If it is not needed, we should remove it.

Can you please double-check this?

Forestsoft-de · 2022-10-03T10:31:44Z

Hey @Forestsoft-de 👋

First of all: Thank you very much for your PRs, we appreciate this a lot 😊

I've decided to first focus on this one, as this is the easier one, and the other one needs a little bit more of discussion. I hope this is fine for you.

I also guess that it could rely on different versions between Debian and alpline git but it didnt. The behavior was reproduceable with the debian container.

I've reviewed your changes, and left a few comments. Could you please take care of them?

In addition it would be great if we could have a test that reproduces the faulty behavior. Can you please add one?

Yeah sure but you know I am the PHP guy :D
I ran an debugging session with my repo and i detected that the repository.Log(&git.LogOptions{From: head.Hash()}) returns only merged commits. I saw that one of the merge commit has also a tag:

For example
git log --oneline

d623a86 (HEAD -> main, origin/main, origin/HEAD) Merge pull request #10 from ForestsoftGmbH/feature/manual_deployment
6985e3a (origin/feature/manual_deployment) fix: make it possible to redeploy an exisisting release
eb2d6b7 Trigger pipeline manually for new version
e867e66 (tag: 0.1.15) Merge pull request #9 from ForestsoftGmbH/bugfix/newversion

In this case only d623a86 and e867e66 are evaluated in the GetConventionalCommitTypesSinceLastRelease function.

Unfortunately i didnt know how to reproduce this behavior in the testcase because you define the history as you expect :)

Have a nice day,

Golo

goloroden · 2022-10-20T18:23:10Z

To somehow move this forward – could you maybe provide a repository that we can use to reproduce the behavior?

Right now I'm not sure how to move on from the current status quo, since we can't reproduce things, and can not exactly tell which effect it will have if we merge this.

Forestsoft-de · 2022-10-21T06:54:51Z

@goloroden sure. I setuped an test repository and implemented the same workflow as i use for my private projects:

https://github.com/ForestsoftGmbH/tnw-semver-tester/actions/runs/3295316981/jobs/5433820444

In this action i would expect the versio 0.2.1 because we have one commit with feat: and one with fix.

The result is 0.1.0 as it would before in the tagged commit on main branch.

BR
Sebastian

goloroden · 2022-10-21T06:58:31Z

Hi @Forestsoft-de 👋

Thanks a lot for setting up the example repository.

Actually, the current behavior is correct:

Since no tag was given in the beginning, get-next-version starts with an assumed version of 0.0.0.
Then you merged a feat and a fix, which basically means minor and patch.
Since minor are allowed to include patches, the fix is not relevant here, only the feat is considered.
Hence the minor version is increased, and you get a 0.1.0.

So, it behaves as expected (and conforming to the way Semver works).

goloroden · 2022-10-21T06:59:37Z

PS: Since the software works as designed, I'm going to close this PR.

Forestsoft-de · 2022-10-21T07:06:09Z

Hi @Forestsoft-de 👋

Thanks a lot for setting up the example repository.

Actually, the current behavior is correct:

Since no tag was given in the beginning, get-next-version starts with an assumed version of 0.0.0.
Are you sure?

git log --oneline

9d039d5 (HEAD -> fix/deprecated-warning, origin/fix/deprecated-warning) feat: improve the documentation
c5f9087 fix: documentation
2076428 (origin/main, origin/HEAD, main) Merge pull request #1 from ForestsoftGmbH/feature/github-action
0bf9f5c (tag: 0.1.0, origin/feature/github-action) fix: version constraint
38fb3fc feat: releasing this repository using action
e6428b4 Initial release

Then you merged a feat and a fix, which basically means minor and patch.

Since minor are allowed to include patches, the fix is not relevant here, only the feat is considered.

Then i would expect the version 0.2.0 :)

Hence the minor version is increased, and you get a 0.1.0.

So, it behaves as expected (and conforming to the way Semver works).

BR
Sebastian

goloroden · 2022-10-21T07:07:38Z

Then i would expect the version 0.2.0 :)

Why? A minor from 0.0.0 is a 0.1.0, or am I missing something?

Forestsoft-de · 2022-10-21T07:15:33Z

Check the gitlog. There is an tag on a previous commit in main branch.

And i saw it in the debugger that some commits are not handled by the get-next-version code. That was the intention of this PR.

BR
Sebastian

goloroden · 2022-10-21T07:31:44Z

The 0.1.0 tag is on the third commit, and after that there is only the merge commit, which is missing a prefix, hence no new version is being generated.

Somehow I have the feeling that we are at cross purposes … from what I am able to understand, the history is as follows:

Initlal commit
feat commit
fix commit
Tag 0.1.0
Merge commit

Was the tag created manually, or using get-next-version?
And, at what point in time do you now try to run get-next-version and expect the 0.2.0?

Because, up to 0.1.0 there was a feat and a fix, and no previous tag, so 0.1.0 is correct.
And after the 0.1.0 tag there are no commits with any prefix, so they are ignored.

Forestsoft-de · 2022-10-21T10:01:49Z

The 0.1.0 tag is on the third commit, and after that there is only the merge commit, which is missing a prefix, hence no new version is being generated.

Somehow I have the feeling that we are at cross purposes … from what I am able to understand, the history is as follows:

Initlal commit

feat commit

fix commit

Tag 0.1.0

Merge commit

The action is running on the fix/deprecated-warning branch there is an active pull request to the main branch.
On top of that last merge commit there are 2 commits:

9d039d5 (HEAD -> fix/deprecated-warning, origin/fix/deprecated-warning) feat: improve the documentation
c5f9087 fix: documentation

If i push changes to this branch the tag is included before and as you say it should only raise the minor version to 0.2.0

Pushed a new change here:
ForestsoftGmbH/tnw-semver-tester@ffac773

Also with feat: prefix and the version gets again 0.1.0

Was the tag created manually, or using get-next-version? And, at what point in time do you now try to run get-next-version and expect the 0.2.0?

The tag was created manually.

Because, up to 0.1.0 there was a feat and a fix, and no previous tag, so 0.1.0 is correct. And after the 0.1.0 tag there are no commits with any prefix, so they are ignored.

Yeah you are right for the main branch but the action was executed on the fix/deprecated-warning branch.

BR
Sebastian

goloroden · 2022-10-21T12:26:55Z

Okay, thanks for clarifying this 😊

My first approach was to clone your repository, switch to the branch you mentioned, and run get-next-version locally. As expected, the result is 0.2.0:

$ git clone git@github.com:ForestsoftGmbH/tnw-semver-tester.git
$ git checkout fix/deprecated-warning
$ get-next-version
0.2.0

So, apparently the current version of the tool performs the right thing.
The question is: Why does it behave differently on GitHub?

PS: Can you please give this a try as well, just to double-check that I didn't to anything wrong?

Forestsoft-de · 2022-10-21T12:57:50Z

@goloroden you should check it with the docker container you provide in the action. Maybe there is something different.

As i mentionied with my patch it works as expected. I already use it in multiple of my pipelines with an own fixed docker container:

https://hub.docker.com/repository/docker/forestsoft/get-next-version

BR
Sebastian

goloroden · 2022-10-21T13:22:34Z

I agree that there is something wrong, but if it works correctly locally, the issue is not in the code of the tool, but rather inside how we build / use the Docker image, is it?

So, I'm not sold yet on the code change, because apparently it also works correctly without this change, just not in the Dockerized situation.

And we need to figure out what causes the problem there.

goloroden · 2022-10-22T05:52:50Z

The bug is also not within the Docker image.

I built the Docker image locally, and ran it against your repository, and it also produces 0.2.0.
Hmmm 🤔

The only thing that remains then is the way the workflow is written that uses get-next-version.

According to our documentation, you did everything correctly (especially set the fetch-depth parameter), but we have only ever tested this on the main branch.

My current guess is that (for somewhat reason) your workflow is running on the wrong branch … but that's just a gut feeling, and I can not yet explain it. I'll get back to you once I have new insights.

goloroden · 2022-10-22T05:57:48Z

Okay, I've got the solution (I guess): actions/checkout#648

When using fetch-depth: 0 the current branch is ignored, and you always get the main branch.
You need to specify the desired branch / commit by providing ref: <...>.

As said, this did never catch our eye before since we only ever run this on the main branch.

Can you please try to provide the correct ref, and see if this fixes the issue?

Forestsoft-de · 2022-10-22T14:37:30Z

Hey @goloroden

the ref: ${{ github.event.pull_request.head.sha }} results in:

{"level":"fatal","time":1666449201,"message":"object not found"}

See: https://github.com/ForestsoftGmbH/tnw-semver-tester/actions/runs/3303603725/jobs/5451727442

BR
Sebastian

goloroden · 2022-10-22T17:30:50Z

I've opened a PR on your repository by myself, to make some tests – could you maybe allow it to run? See ForestsoftGmbH/tnw-semver-tester#3

Forestsoft-de · 2022-10-22T21:44:55Z

@goloroden it works now.

Ive updated the PR to only improved example in your readme.

BR
Sebastian

Forestsoft-de requested review from dotKuro, goloroden, strangedev and yeldiRium as code owners October 2, 2022 23:48

goloroden self-assigned this Oct 3, 2022

goloroden requested changes Oct 3, 2022

View reviewed changes

goloroden mentioned this pull request Oct 3, 2022

Release Improvements #47

Closed

goloroden closed this Oct 21, 2022

goloroden reopened this Oct 21, 2022

chore: improve documentation to be able to bump pull requests correctly

10a108b

Forestsoft-de force-pushed the fix/commit-history branch from ea1cc77 to 10a108b Compare October 22, 2022 21:44

Merge branch 'main' into fix/commit-history

bdb2adc

goloroden approved these changes Oct 23, 2022

View reviewed changes

goloroden merged commit 3acd5fa into thenativeweb:main Oct 23, 2022

Forestsoft-de deleted the fix/commit-history branch October 23, 2022 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Not all commits beeing evalutated, added all option to gitlog #48

fix: Not all commits beeing evalutated, added all option to gitlog #48

Forestsoft-de commented Oct 2, 2022

goloroden left a comment

goloroden Oct 3, 2022

goloroden Oct 3, 2022

Forestsoft-de commented Oct 3, 2022

goloroden commented Oct 20, 2022

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022

goloroden commented Oct 21, 2022

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022 •

edited

Loading

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022 •

edited

Loading

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022

goloroden commented Oct 22, 2022

goloroden commented Oct 22, 2022

Forestsoft-de commented Oct 22, 2022

goloroden commented Oct 22, 2022

Forestsoft-de commented Oct 22, 2022 •

edited

Loading

fix: Not all commits beeing evalutated, added all option to gitlog #48

fix: Not all commits beeing evalutated, added all option to gitlog #48

Conversation

Forestsoft-de commented Oct 2, 2022

goloroden left a comment

Choose a reason for hiding this comment

goloroden Oct 3, 2022

Choose a reason for hiding this comment

goloroden Oct 3, 2022

Choose a reason for hiding this comment

Forestsoft-de commented Oct 3, 2022

goloroden commented Oct 20, 2022

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022

goloroden commented Oct 21, 2022

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022 • edited Loading

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022 • edited Loading

Forestsoft-de commented Oct 21, 2022

goloroden commented Oct 21, 2022

goloroden commented Oct 22, 2022

goloroden commented Oct 22, 2022

Forestsoft-de commented Oct 22, 2022

goloroden commented Oct 22, 2022

Forestsoft-de commented Oct 22, 2022 • edited Loading

goloroden commented Oct 21, 2022 •

edited

Loading

goloroden commented Oct 21, 2022 •

edited

Loading

Forestsoft-de commented Oct 22, 2022 •

edited

Loading