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

refactor to composite action #62

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

renefritze
Copy link
Contributor

No description provided.

@platisd
Copy link
Owner

platisd commented Jan 12, 2024

Good work. Do you have a link to a successful run?

Also, from what I understand, the users don't need to change anything when they use the Action. It will run in whatever environment they have launched, correct?

@renefritze
Copy link
Contributor Author

renefritze commented Jan 12, 2024

Good work. Do you have a link to a successful run?

Thanks. I've only tested it on a private project. I'll have a think on how this could be generally tested in CI here.

Also, from what I understand, the users don't need to change anything when they use the Action. It will run in whatever environment they have launched, correct?

Generally, yeah. If they are already setting up python, they'd need to pass in their version, else the setup-action will clash.

@@ -27,7 +27,7 @@ if [ ! -f "$INPUT_CLANG_TIDY_FIXES" ]; then
exit 0
fi

/action/run_action.py \
${GITHUB_ACTION_PATH}/run_action.py \
Copy link
Contributor

@oleg-derevenetz oleg-derevenetz Jan 12, 2024

Choose a reason for hiding this comment

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

Hi @renefritze what if GITHUB_ACTION_PATH contains a space or a special character like ? or * - here and in the action.yml?

Copy link
Owner

Choose a reason for hiding this comment

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

Has this been addressed @renefritze btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quoted the paths, yes.

@platisd
Copy link
Owner

platisd commented Jan 12, 2024

Generally, yeah. If they are already setting up python, they'd need to pass in their version, else the setup-action will clash.

@renefritze 🤔 can we do anything to avoid a new major version, since this is a breaking change?

@platisd platisd linked an issue Jan 14, 2024 that may be closed by this pull request
@platisd
Copy link
Owner

platisd commented Jan 14, 2024

Generally, yeah. If they are already setting up python, they'd need to pass in their version, else the setup-action will clash.

@renefritze 🤔 can we do anything to avoid a new major version, since this is a breaking change?

Some follow-up questions I just thought:

  • What are the consequences of such "clash"? That after running this action the user will end up with a potentially different Python version than the one they had set up?
  • If there's no way to avoid this clash, what's the best way to make it very clear to the users this may happen and therefore they need to explicitly set the python version?
    • Potentially make the Python version of this action a required argument?

@renefritze
Copy link
Contributor Author

Some follow-up questions I just thought:

  • What are the consequences of such "clash"? That after running this action the user will end up with a potentially different Python version than the one they had set up?

I don't know. From the docs on setting multiple versions, I would think the last version "wins", yeah.

  • Potentially make the Python version of this action a required argument?

That would work, yes.
I might just have had a better idea that isolates this action's setup entirely: 51a21cb

@platisd
Copy link
Owner

platisd commented Jan 15, 2024

Some follow-up questions I just thought:

  • What are the consequences of such "clash"? That after running this action the user will end up with a potentially different Python version than the one they had set up?

I don't know. From the docs on setting multiple versions, I would think the last version "wins", yeah.

  • Potentially make the Python version of this action a required argument?

That would work, yes. I might just have had a better idea that isolates this action's setup entirely: 51a21cb

${GITHUB_ACTION_PATH}/venv/python -m pip install -r ${GITHUB_ACTION_PATH}/requirements.txt

Did this work?

@platisd
Copy link
Owner

platisd commented Jan 15, 2024

Also, can you please rebase so this sits on top of #63 and therefore we get the new workflow triggered?

@renefritze
Copy link
Contributor Author

Some follow-up questions I just thought:

  • What are the consequences of such "clash"? That after running this action the user will end up with a potentially different Python version than the one they had set up?

I don't know. From the docs on setting multiple versions, I would think the last version "wins", yeah.

  • Potentially make the Python version of this action a required argument?

That would work, yes. I might just have had a better idea that isolates this action's setup entirely: 51a21cb

${GITHUB_ACTION_PATH}/venv/python -m pip install -r ${GITHUB_ACTION_PATH}/requirements.txt

Did this work?

Not right away :)
https://github.com/platisd/clang-tidy-pr-comments/actions/runs/7531944101/job/20501597277?pr=62

action.yml Show resolved Hide resolved
@renefritze
Copy link
Contributor Author

Noticed I still had a !fixup commit in here, squashed that and force pushed.

@platisd platisd force-pushed the refactor_to_composite_action branch 2 times, most recently from f0c25d8 to 7e6f047 Compare January 15, 2024 17:56
@platisd
Copy link
Owner

platisd commented Jan 15, 2024

Sorry for the force pushing. Tested out the PR here.

@platisd platisd force-pushed the refactor_to_composite_action branch from 7e6f047 to c530773 Compare January 15, 2024 18:07
@platisd
Copy link
Owner

platisd commented Jan 15, 2024

OK, I squashed some commits to make it more clear what is being done in this PR, hope that's fine with your @renefritze

@renefritze
Copy link
Contributor Author

OK, I squashed some commits to make it more clear what is being done in this PR, hope that's fine with your @renefritze

Sure thing.

@platisd platisd merged commit dc994a2 into platisd:master Jan 15, 2024
2 checks passed
@platisd
Copy link
Owner

platisd commented Jan 15, 2024

Thanks for your efforts, it's now included in the latest release! 🎉

@renefritze
Copy link
Contributor Author

Awesome, thank you very much for working with me.

@renefritze renefritze deleted the refactor_to_composite_action branch January 24, 2024 10:27
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.

non-docker option
3 participants