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.

Sign up for GitHub

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

Jump to bottom

Add 'infile' option #220

Merged
merged 2 commits into from
Jun 22, 2023
Merged

Add 'infile' option #220

merged 2 commits into from
Jun 22, 2023

Conversation

binomialstew
Copy link
Contributor

As discussed here. I still need to test this.

@binomialstew binomialstew mentioned this pull request Jun 15, 2023
Closed
TriPSs
TriPSs reviewed Jun 15, 2023
View reviewed changes
README.md Outdated
@@ -13,6 +13,7 @@ This action will bump version, tag commit and generate a changelog with conventi
- **Optional** `git-branch`: The branch used to push. Default is the current branch (`${{ github.ref }}`)
- **Optional** `git-url`: Git repository domain. Default is `github.com`
- **Optional** `git-path`: Path filter for the logs. If set, only commits that match the path filter will be considered. By default, we won't use this feature(empty string).
- **Optional** `infile`: Read the CHANGELOG from this file.
Copy link
Owner

Choose a reason for hiding this comment

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

Could we bring this one inline with output-file? So input-file, maybe also describe a bit more what it does? If I understood correct :Read the CHANGELOG from this file and prepend the new release to it.

TriPSs
TriPSs reviewed Jun 15, 2023
View reviewed changes
action.yml Outdated
@@ -56,6 +56,11 @@ inputs:
default: "v"
required: false

infile:
description: "Read the CHANGELOG from this file"
default: "CHANGELOG.md"
Copy link
Owner

Choose a reason for hiding this comment

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

Also wondering what happens here? Wil this now not cause the whole old changelog to be append to the new one? (if you generate more then only the latest release, 5 is currently the default)

Think it's better to not have a default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. In my tests, it retains the prior content of the CHANGELOG.md as long as it is set as --infile—so all prior releases. This means that release-count will not consider any releases in input-file. I will add a note about this to release-count

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK updated

@binomialstew
Copy link
Contributor Author

@TriPSs, I've addressed your comments

@binomialstew binomialstew force-pushed the feature/infile-addition branch 12 times, most recently from c3fcce9 to 5567f8d Compare June 16, 2023 00:46
@binomialstew
Copy link
Contributor Author

@TriPSs, it looks like the infile option is exclusive to command line usage. I'm testing some things now and will update this when possible.

@binomialstew binomialstew force-pushed the feature/infile-addition branch 11 times, most recently from d123779 to 90f4322 Compare June 16, 2023 01:34
@binomialstew binomialstew force-pushed the feature/infile-addition branch 20 times, most recently from 6c31de0 to ee93fd7 Compare June 16, 2023 22:20
@binomialstew
Copy link
Contributor Author

binomialstew commented Jun 16, 2023 •
edited

OK. This has been updated, and I tested the action in my own project to verify:

  • That all releases are maintained (tested 7 releases) when using input-file option
  • That only the default number of releases are maintained (5) when neither input-file or release-count are used
  • If release-count is set, the releases are limited to that number

@TriPSs
Copy link
Owner

TriPSs commented Jun 21, 2023

Thanks for the PR! Could you also add a test case for this?

@binomialstew
Copy link
Contributor Author

Thanks for the PR! Could you also add a test case for this?

OK. Test case is added. Let me know if this is in line with what you were thinking.

@TriPSs
Copy link
Owner

TriPSs commented Jun 22, 2023

Thanks for the PR!

@TriPSs TriPSs merged commit 69dad78 into TriPSs:releases/v3 Jun 22, 2023
25 checks passed
@binomialstew
Copy link
Contributor Author

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TriPSs TriPSs TriPSs left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@binomialstew @TriPSs