Skip to content

Conversation

@thejeff77
Copy link

What this PR does / Why we need it

Adds two new optional variables:

  • ref: In case you want to target something other than HEAD
  • force: In case you want to move the tag. Great for walking tags.

Which issue(s) this PR fixes

Fixes #
#13
#12

| `tag` | A Git tag name. | `string` | `true` | `N/A` |
| `message` | A message for the Git tag. | `string` | `false` | `''` |
| `ref` | Commit SHA or tag to target. | `string` | `false` | `''` |
| `force` | A message for the Git tag. | `boolean`| `false` | `false` |
Copy link

Choose a reason for hiding this comment

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

Fix description

Choose a reason for hiding this comment

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

Please fix desciption of the force paramater

| `message` | A message for the Git tag. | `string` | `false` | `''` |
| `ref` | Commit SHA or tag to target. | `string` | `false` | `''` |
| `force` | A message for the Git tag. | `boolean`| `false` | `false` |
| `always-pass`| If step should pass if tag push fails | `boolean`| `false` | `false` |
Copy link

Choose a reason for hiding this comment

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

always-pass sounds weird. ignore-failures maybe? Or always-succeed?

id: get-latest-tag

- uses: actions-ecosystem/action-bump-semver@v1
- uses: thejeff77/action-bump-semver@v1.0.0
Copy link

Choose a reason for hiding this comment

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

Use actions-ecosystem

Choose a reason for hiding this comment

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

+1 about use of actions-ecosystem
If you do a PR to original repo it should stay as is. In your own repo feel free to override it.

level: minor

- uses: actions-ecosystem/action-push-tag@v1
- uses: thejeff77/action-push-tag@v1.0.0
Copy link

Choose a reason for hiding this comment

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

Use actions-ecosystem

Choose a reason for hiding this comment

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

Same here. +1 about use of actions-ecosystem

Comment on lines +1 to +3
name: Push Any Git Tag
description: based on Actions Ecosystem Push Tag - Fixed with additional features.
author: Actions Ecosystem & theJeff77
Copy link

Choose a reason for hiding this comment

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

I wouldn't change these. Nor do you need to put your name in as the author just because you PR.

Choose a reason for hiding this comment

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

+1

Comment on lines +29 to +30
using: "composite"
steps:
Copy link

Choose a reason for hiding this comment

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

I feel like this is an unnecessary over reach on this PR. Just leave it in the Dockerfile and make your changes in the entrypoint.sh.

Copy link

@mikesigs mikesigs left a comment

Choose a reason for hiding this comment

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

I am not the owner of this repo, but I am interested in your PR so I gave it a review.

@mikesigs
Copy link

mikesigs commented Mar 2, 2023

@micnncim This would be a nice addition.

Copy link

@vlavrynovych vlavrynovych left a comment

Choose a reason for hiding this comment

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

  1. Don't replace owner's info like action name, desc, author
  2. Fix description of the force parameter
  3. Add examples for the new parameters

id: get-latest-tag

- uses: actions-ecosystem/action-bump-semver@v1
- uses: thejeff77/action-bump-semver@v1.0.0

Choose a reason for hiding this comment

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

+1 about use of actions-ecosystem
If you do a PR to original repo it should stay as is. In your own repo feel free to override it.

level: minor

- uses: actions-ecosystem/action-push-tag@v1
- uses: thejeff77/action-push-tag@v1.0.0

Choose a reason for hiding this comment

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

Same here. +1 about use of actions-ecosystem

| `tag` | A Git tag name. | `string` | `true` | `N/A` |
| `message` | A message for the Git tag. | `string` | `false` | `''` |
| `ref` | Commit SHA or tag to target. | `string` | `false` | `''` |
| `force` | A message for the Git tag. | `boolean`| `false` | `false` |

Choose a reason for hiding this comment

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

Please fix desciption of the force paramater

| ------------ | -------------------------------------- | -------- | -------- | ------- |
| `tag` | A Git tag name. | `string` | `true` | `N/A` |
| `message` | A message for the Git tag. | `string` | `false` | `''` |
| `ref` | Commit SHA or tag to target. | `string` | `false` | `''` |

Choose a reason for hiding this comment

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

It will be nice to update example in the README.MD with an real case where you use all the new parameters.
For example: it's not clear if the force parameter is analog of git force or it's something else. And why would I need a ref

Comment on lines +1 to +3
name: Push Any Git Tag
description: based on Actions Ecosystem Push Tag - Fixed with additional features.
author: Actions Ecosystem & theJeff77

Choose a reason for hiding this comment

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

+1

type: string
required: false
force:
description: Whether or not to force push (overwrite) the tag if it already exists. Useful for walking tags.

Choose a reason for hiding this comment

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

This descriptions are not available for me when I read the docs or trying to see it will work for me

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants