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

github actions: don't use tj-actions/changed-files #13748

Merged
merged 24 commits into from Nov 23, 2022

Conversation

ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Oct 25, 2022

My motivation for not using tj-actions/changed-files actions any more is that:

  • it breaks its interface (parameters) often, most release are major releases
  • it documents no migration path between major versions
  • breaking changes are not clearly identified

And we cannot stay on tj-actions/changed-files/v20 for long because it uses deprecated features which will be removed soon (cf https://github.com/conan-io/conan-center-index/actions/runs/3326299661/jobs/5499882522#step:4:99 https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/)

This PR demonstrates that for CCI's use case (getting files changed in a pull-request), 7 lines of python code are sufficient to do the job, so it feels to me that it's a smaller maintenance burden to use our custom action than tj-actions/changed-files


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the conan-center hook activated.

@ericLemanissier ericLemanissier marked this pull request as draft October 25, 2022 15:00
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ericLemanissier ericLemanissier marked this pull request as ready for review November 15, 2022 11:24
@prince-chrismc
Copy link
Contributor

I'd like to propose a test.

I want to merge this into my repos master branch. Then have someone without writing access to open a few PRs - We can steal real from from CCI - and make sure we get the correct results.

This is the closest I can think of for a real world test.

Thoughts?

@danimtb
Copy link
Member

danimtb commented Nov 16, 2022

I'd like to propose a test.

I want to merge this into my repos master branch. Then have someone without writing access to open a few PRs - We can steal real from from CCI - and make sure we get the correct results.

This is the closest I can think of for a real world test.

Thoughts?

ok, let's do that

@prince-chrismc
Copy link
Contributor

prince-chrismc@46bbd46

You should be able to make PRs against my repo :)

also random changes like docs, configs, linters -- this should be a good test 🤞

@prince-chrismc
Copy link
Contributor

Opened prince-chrismc#44 so far so good!


I have one small quibble about the order of calling setup-python twice

  • we can remove the one in the action assume its there (which it is by default on the runners)
  • fix the order in the workflow (but we need to make sure to get it right and thats error prone)

image

@ericLemanissier
Copy link
Contributor Author

ericLemanissier commented Nov 17, 2022

I don't think it's worth the risk, considering it literally takes 0 seconds to install python.
There is another problem though: only one file of each type is processed, even if each of my PRs modify two of each. I'll take a look later today

EDIT: OK, it's not a change actually it's simply because the linter stops at the first error.
There is another actual problem though: path matching uses fnmatch, which considers * as any character, any number of time, including path separators. The consequence is that the test recipe fall in the recipe bucket too. I'll see if python's glob does what we want

python's fnmatch treats * as any character any number oftime, including /.
This is not coherent with bash, which excludes / from *.
The fix is to split the pattern and filenames by / and call fnmatch
on each part.
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ericLemanissier
Copy link
Contributor Author

it looks ok now. see how https://github.com/conan-io/conan-center-index/actions/runs/3487128109/jobs/5834387846 did not lint any file (because it only lints recipes), but https://github.com/conan-io/conan-center-index/actions/runs/3487128109/jobs/5834387983 linted the expected test recipe.

Can you pull this change in your fork @prince-chrismc ?

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

Changes not allowed in build 31:

[.github/actions/pr_changed_files/action.yml, .github/workflows/linter-conan-v2.yml, .github/workflows/linter-yaml.yml, recipes/aeron/all/test_package/conanfile.py]

Changing recipes and configuration/docs files in the same pull-request is not allowed. Please, split changes into several pull-requests.

@prince-chrismc
Copy link
Contributor

changes are in and I triggered the PRs again 👍

Maybe next week we can get on a Zoom for an hr and just knock this out? it will be slow going back and forth

@prince-chrismc
Copy link
Contributor

The two PRs look really promising. They ran only every necessary so 👍

I'd like to see a docs PR (aka no files changes) and a linter PR just to make sure those two workflows still work 🙏 than I think we can green lite this to go in

@ericLemanissier
Copy link
Contributor Author

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Everything checks out and looks to be running correctly from a fork 🤞

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 83dfd13 into conan-io:master Nov 23, 2022
@ericLemanissier ericLemanissier deleted the patch-5 branch November 23, 2022 14:58
@ericLemanissier ericLemanissier mentioned this pull request Nov 23, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants