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

chore: upgrade libs, node 20 #16

Closed
wants to merge 1 commit into from
Closed

chore: upgrade libs, node 20 #16

wants to merge 1 commit into from

Conversation

sammcj
Copy link

@sammcj sammcj commented Nov 15, 2023

Upgraded repo to support current stable nodeJS and libraries.

  • node 16 -> node 20
  • actions-github v5 -> v6
  • corrected input types (true != "true")

@iamludal
Copy link

Hi @aurelien-baudet , any updates on this PR please? Thanks.

Copy link
Collaborator

@aurelien-baudet aurelien-baudet left a comment

Choose a reason for hiding this comment

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

It seems ok, just a question

@@ -23,7 +23,7 @@ inputs:
display-workflow-run-url:
description: 'Get the URL of the triggered workflow and display it in logs (useful to follow the progress of the triggered workflow)'
required: false
default: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you add quotes ? It should be a boolean value

Copy link

@figroc figroc Feb 15, 2024

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really, why ?

Choose a reason for hiding this comment

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

It should be a boolean value

Looking at the schema, the type of default is string. However, I don't think it will actually change any behaviour as GHA likely coerces it into a string anyway. This StackOverflow answer may also help explain.

It may be worth adding the quotes if only to avoid your editor showing a syntax error, but it should probably then use single-quotes to be consistent with the rest of the file.

Should this line be changed?

I also don't think this line should be changed. Whether the input is processed as a string or boolean is dependent on whether core.getInput or core.getBooleanInput is used in the script: https://www.npmjs.com/package/@actions/core#inputsoutputs

@@ -35,22 +35,22 @@ inputs:
wait-for-completion:
description: 'Block until the triggered workflow has finished'
required: false
default: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, why ?

Copy link

@figroc figroc Feb 15, 2024

Choose a reason for hiding this comment

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

@VincentLanglet
Copy link

What's the status of the PR ?

Also, there is a warning with the action:

Warning: The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

@MugishaU
Copy link

Also wondering if there is any update on this?

@aurelien-baudet
Copy link
Collaborator

Sorry, I have no more time and energy to maintain this repository. I even don't have computer anymore. Someone of you should for this repository and merge the different pull requests.
Tell me who creates the fork and I will archive this repository but indicate which repository follows up

@iamludal
Copy link

Hi @aurelien-baudet, I have created a fork of your repository that I can maintain. https://github.com/iamludal/workflow-dispatch

@VincentLanglet
Copy link

Hi @aurelien-baudet, I have created a fork of your repository that I can maintain. iamludal/workflow-dispatch

Does this project really need another fork when there is already the original repository ?
https://github.com/benc-uk/workflow-dispatch

The original repository seems still maintained. Is there a reason to not for moving to it ?

@iamludal
Copy link

iamludal commented May 14, 2024

Hi @aurelien-baudet, I have created a fork of your repository that I can maintain. iamludal/workflow-dispatch

Does this project really need another fork when there is already the original repository ? https://github.com/benc-uk/workflow-dispatch

The original repository seems still maintained. Is there a reason to not for moving to it ?

Yes, this fork from @aurelien-baudet adds some inputs to wait for the workflow completion before exiting. In the original repository, the workflow is triggered and exits successfully immediately.

@VincentLanglet
Copy link

VincentLanglet commented May 14, 2024

Yes, this fork from @aurelien-baudet adds some inputs to wait for the workflow completion before exiting. In the original repository, the workflow is triggered and exits successfully immediately.

Thanks for the answer.

IMHO the best, if possible, would be to add those inputs in the original repository which are well maintained.

I'll ask the original repo if he's interested benc-uk#73

@iamludal
Copy link

iamludal commented May 15, 2024

Yes, this fork from @aurelien-baudet adds some inputs to wait for the workflow completion before exiting. In the original repository, the workflow is triggered and exits successfully immediately.

Thanks for the answer.

IMHO the best, if possible, would be to add those inputs in the original repository which are well maintained.

I'll ask the original repo if he's interested benc-uk#73

I agree with you, I would prefer this solution over having to maintain a fork myself. I can do it if needed, but this is not the best solution.

@aurelien-baudet
Copy link
Collaborator

@fbiesse
Copy link
Collaborator

fbiesse commented Jun 21, 2024

These issues were resolved in #18

@fbiesse fbiesse closed this Jun 21, 2024
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.

8 participants