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

Allow use of last git commit message or a custom message #94

Closed
wants to merge 8 commits into from
Closed

Allow use of last git commit message or a custom message #94

wants to merge 8 commits into from

Conversation

richard-muvirimi
Copy link
Contributor

@richard-muvirimi richard-muvirimi commented May 29, 2022

Description of the Change

Allows customization of svn commit message using the last git message or any other text, defaulting to the commit message "Update to version $VERSION from GitHub". I understand not everyone wants the same message across different plugins, becomes pointless to even have the option to set the message on svn's part.

Partially addresses #18

Alternate Designs

Not sure there was any other alternative besides forking the repository and changing to a message one desires, and in the process set yourself up to have to maintain the forked code

Possible Drawbacks

Different messages for each commit (Commit messages can be different?: Mind Blown) ;), On a serious note, commit messages should be different across releases so that it's easier to identify what changed.

Verification Process

I created a dot sh file that I ran to test all the possible paths (pasted below)

INPUT_COMMIT_MESSAGE="Custom commit message"

if [[ "$INPUT_COMMIT_MESSAGE" != false ]]; then
	if [[ "$INPUT_COMMIT_MESSAGE" = true ]]; then
		#.git DIR still in place, so we should be able to retrieve the last message
		MESSAGE=$(git -C "$GITHUB_WORKSPACE" log -1 --format=%s)
	else
		#provided commit message
		MESSAGE=$INPUT_COMMIT_MESSAGE
	fi
else
	MESSAGE="Update to version $VERSION from GitHub"
fi

echo $MESSAGE

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Credits

Partially addresses #18

@jeffpaul jeffpaul requested a review from dinhtungdu June 2, 2022 03:27
@jeffpaul jeffpaul added this to the Future Release milestone Jun 2, 2022
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

As far as I understand the issue, we want the commit message to be more explicit so authors publishing multiple plugins can distinguish which plugins are updated easier. I'm not sure how helpful the last commit is in this case. Personally, I never include the plugin name or repo name in the commit message.

If everything we want is a better way for plugin authors to keep track of deployments, then I think including the plugin name or the repo name in the current commit message is enough. By doing so we also avoid adding extra flags which makes our action more complicated.

IMHO, @jeffpaul's suggestion works best for me

Update to version $VERSION from GitHub for $PLUGIN_NAME

What do you think @richard-muvirimi?

@richard-muvirimi
Copy link
Contributor Author

richard-muvirimi commented Jun 2, 2022

@dinhtungdu

This PR was not directly addressing the issue attached, but offering an alternative to the svn commit message used. Normally when using svn directly, one would provide different messages for each plugin release, maybe a summary of the changelog, and some, usually right after committing to GitHub, might then use the same message for an svn commit.

Reviewing svn commits, later on, one would expect to view a log like this for Woocommerce and this for Hello Dolly with maybe a short summary of what changed.

Though using a workflow, one should be able to recognize their changes just from the message, I believe that is the reason we are able to provide the message in the first place.

We might have some prefix text before the message for git, and provide the git message as an option defaulting to the message you suggested if one opts out.

Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

@richard-muvirimi This makes sense to me now, thanks for your explanation! I left some minor comments, please take a look.

Richard Muvirimi and others added 2 commits June 3, 2022 08:22
You might also want to remove the extra git from Use git the latest git...

Co-authored-by: Tung Du <[email protected]>
Copy link
Contributor Author

@richard-muvirimi richard-muvirimi left a comment

Choose a reason for hiding this comment

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

Have removed the extra git from your suggested change. Use of tables should be able to better convey what each field takes which i have provided examples of. Use of a switch statement, might have been a better approach for resolving the message to use.

Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

@richard-muvirimi Thanks for the update! I added some more comments to your PR, please take another look!

Richard Muvirimi and others added 4 commits June 4, 2022 19:24
@dinhtungdu
Copy link
Contributor

It is possible to set a dynamic commit message either using output from another action or setting an environment variable before running this action.

As you mentioned in another PR, it's complex to set the env variables, what can we do to make setting the dynamic message easier?

Modifying Github environment variables are more of a hack and require good knowledge of the bash language. Github might one day prevent modifying the environment from outside action context, as they currently do with destructive file actions.


From another perspective, I think we can be more explicit here by using two flags:

  • use-git-commit-message: Use the latest git commit message if true.
  • custom-commit-message: Highest priority if set, use custom dynamic message for the svn commit.

To make the dynamic message more friendly, we can define common variables like version, plugin slug, plug name, and bundle size...

What do you think?

@dinhtungdu
Copy link
Contributor

Another idea (which is better IMO)

  • use-git-commit-message: Use the latest git commit message if true. Take over commit-message.
  • commit-message: Default to "Update to version $VERSION from GitHub", dynamic by default with some variable like version, plugin slug, plug name, and bundle size available.

@richard-muvirimi
Copy link
Contributor Author

As you mentioned in another PR, it's complex to set the env variables, what can we do to make setting the dynamic message easier?

Not difficult per se, GitHub actually recommends using environment variables (thought it was a hack ¯\(ツ)/¯ ). Though still not sure most users would be familiar with setting environment variables (Just my wild assumption).

To make it easier, I think your suggestion

To make the dynamic message more friendly, we can define common variables like version, plugin slug, plug name, and bundle size...

is better. At first, I had done a commit that would resolve bash code but seems, GitHub had to remove similar functionality due to security implications involved with that.

From another perspective, I think we can be more explicit here by using two flags:

This is not a show stopper on my end but had a minor suggestion. The purpose of using one input was so that users would not provide both options. We could change the provided values to default for the existing message or git for the git message.

or preferably

Since we might be allowing the use of a dynamic message, would it not be better to have one input taking two options

  1. git git commit message
  2. the actual text if not provided will default to the current message still going through the variable replacements

the pseudocode being

if $COMMIT_MESSAGE = "git"; then
  MESSAGE=$(git -C "$GITHUB_WORKSPACE" log -1 --format=%s)
else
  MESSAGE="Update to version $VERSION from GitHub";
  if [[ -n $COMMIT_MESSAGE ]]; then
    MESSAGE=$COMMIT_MESSAGE
  fi

  #replacement code here
fi

A bash function like PHP's strtr would have been handy for the replacements.

The last part is not a show stopper for me, we can have the two inputs.

@richard-muvirimi
Copy link
Contributor Author

@dinhtungdu i might have come off as demanding in my previous comments, but the real purpose for my PR is, i use a different workflow, that is not tag based. I know there are others that might have the same workflow and might not be easy to understand especially if you have not used such. Instead of creating a completely new action, I felt it better to contribute to an existing one but if my contributions are not well received, you may close this PR. Currently my workflows depend on the approval of this PR. All changes brought by this PR, are 100% compatible with existing workflows as they are opt-in based.

@richard-muvirimi richard-muvirimi deleted the commit-message branch June 13, 2022 07:11
@dinhtungdu
Copy link
Contributor

@richard-muvirimi I'm sorry for the late response, I've been busy working on the other project so I didn't reply to you on time. I'm so sorry that it makes you feel your contribution is not well received. We're appreciated your time and effort in contributing to this action and OSS in general!

This is PR is great and I think we can fully solve the #18, that's why I added the comments above.

The purpose of using one input was so that users would not provide both options. We could change the provided values to default for the existing message or git for the git message.
Since we might be allowing the use of a dynamic message, would it not be better to have one input taking two options

After consideration, I agree that adding just one option is better. git for the latest commit and custom message which is default to the Update to version $VERSION from GitHub for $PLUGIN_NAME.

I can't reopen this one so can you create another PR?

@richard-muvirimi
Copy link
Contributor Author

To tell the truth i got frustrated with all the waiting, now almost 15 days since first PR. I understand you have to review every PR but looking at the remaining hanging PRs addressing issues that where later merged through other PRs, i feel you might have to revise your review process. No one was addressing the issues, then someone decides to dedicate time, which is then wasted going back and fourth explaining the same thing over and over when the truth lies in the code. An action/project should not be that much opinionated, it just stunts it's growth, tell me about all the linux distros and wasted brain power

Most are already committing manually however they want, so why should an action care. As long as it does what it is supposed to do. That's why rsync takes all those options if you hadn't realised

With just two PR 's requiring about a month to resolve, cannot begin to imagine how long it would take to add:

  1. tests to prevent regressions, which would require breaking it into smaller components
  2. A way to cleanup tags, using a user selectable cleanup strategy e.g picking highest major versions of past releases, or using git, percentage usage from wordpress, or list of tags to keep
  3. Using a custom svn repository url
  4. Not sure if tags are even commited as the files are copied after the addition into svn
  5. Option to include a folder in generated zip, that way one wont have to worry about the name of the zip
  6. Do away with .distignore and .gitattributes in favour of providing a working directory
  7. Support windows runners, so that self hosted windows runners can still run this action
  8. redesign the documentation, accepted fields (not touching the advertisements)
  9. Use of a custom assets directory

I had started working on another implementation though still needing a bit more work and testing (3 days at time of this comment) addressing most of the points raised above. Wouldn't want to stop only for me to finally have a usable action next year...

@jeffpaul
Copy link
Member

@richard-muvirimi I wanted to jump in here ahead of the rest of the team to better explain things that may not be obvious given your previous comment.

This Action is used by at least 1.6k plugins as reported by GitHub and as you can imagine we need to be rather delicate and precise when updating this Action as many people (including us) rely on it to release plugin updates to WordPress.org. Any change that would break that flow could irreparably harm the code management and release processes for many people, something we do not take lightly. Thus we are going to take out time to ensure any changes take our known and expected use cases into consideration before merging in and releasing something.

Our team currently maintains many other open source repositories, of which this repository is just one, so you will need to forgive the team who are freely supporting all these projects that a specific review on this PR is taking some time as we need to ensure we're giving attention to all our projects (based on their stated support level).

We appreciate your contribution and your time, but given the focus of this project and needs across all the projects we support you will need to patient while our team gets into reviewing this (or any other) PR.

@richard-muvirimi
Copy link
Contributor Author

I had decided to start a new action but realized that's how we end up with multiple projects doing the same thing, solving the same problems, just wasted effort. I will try to answer your concerns about my suggestions as follows:

This Action is used by at least 1.6k plugins as reported by GitHub

I think this can be resolved by the use of semantic versioning. We do not have to archive the action because it now has users.

Our team currently maintains many other open-source repositories, of which this repository is just one

I am not suggesting you resolve issues or PRs when they are raised, as I also only have time on some weekends, but at least an indication that a review is scheduled would be fine. Tests would be a sure way to ensure backward compatibility is maintained. I do not challenge 10up open source projects, but offer a way to embrace different approaches people might have to the same problem.

Embracing different workflows will help make this action non-rigid with room for improvement, even in ways you had not thought of before. Github already has filters for when actions should run, and I do not think we should be further enforcing this by limiting to tags.

I had deleted my fork of the project and will try to restore it so that we continue the merge

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