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

Track feedstock changes in .github #1821

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakirkham
Copy link
Member

Feedstocks can customize the content of .github. However if git ignores these changes, it could result in these files being dropped. So exclude them from .gitignore to ensure git still tracks them

@jakirkham jakirkham requested a review from a team as a code owner January 4, 2024 03:13
@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

Feedstocks can customize the content of .github

That's discouraged right? What are the use-cases?

@jakirkham
Copy link
Member Author

Is it? News to me

We have a global set of GitHub templates that doesn't always make sense. We let people customize these when needed (for example)

@@ -12,6 +12,7 @@
# Don't ignore any files/folders recursively in the following folders
!/recipe/**
!/.ci_support/**
!/.github/**
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
!/.github/**
!/.github/*.md

@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

Ah, I forgot about the templates.

@h-vetinari
Copy link
Member

Aren't we using .github/workflows for implementing e.g. automerge: true?

def render_github_actions_services(jinja_env, forge_config, forge_dir):
# render github actions files for automerge and rerendering services
skip_files = _get_skip_files(forge_config)
for template_file in ["automerge.yml", "webservices.yml"]:
template = jinja_env.get_template(template_file + ".tmpl")
rel_target_fname = os.path.join(".github", "workflows", template_file)

If I change automerge from true to false or vice versa, why shouldn't those changes in .github be checked in on the next rerender? We do this for everything else AFAIK (as a semi-related example, store_build_artifacts: true causes a bunch of code to be checked in upon rerender).

@jakirkham
Copy link
Member Author

No worries

Re-rendering also makes changes to .github/workflows

This came up recently when redeploying the webservice GHA

Maybe we want to capture that a different way

@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

If I change automerge from true to false or vice versa, why shouldn't those changes in .github be checked in on the next rerender?

I don't understand. Can you send me some commands on how you expect things to break?

@h-vetinari
Copy link
Member

There was an error while rerendering https://github.com/conda-forge/libuv-feedstock that @jakirkham tracked down (details in core chat) to

error: Your local changes to the following files would be overwritten by checkout:
	.github/workflows/automerge.yml
Please commit your changes or stash them before you switch branches.

I believe this might actually be a fallout from #1413, because before that PR we did commit things into .github/workflows (example)

@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

Wrong analysis. Restarted the heroku server and rerendering is back to normal.

@h-vetinari
Copy link
Member

Wrong analysis. Restarted the heroku server and rerendering is back to normal.

OK, great if the restart fixed it. Can you describe what caused this, and are you sure it won't happen again (with higher frequency than heroku was failing before)?

From my POV, #1413 has left feedstocks with stuff in .github/workflows in an inconsistent state -- we shouldn't have files checked in that are also .gitignore'd. So either we should remove those files (assuming we can make things work without them), or un-ignore that folder.

@jakirkham
Copy link
Member Author

There are a couple issues that are related:

  1. Re-rendering stopped working (sounds like Heroku stopped working; thanks for figuring that out! 🙏)
  2. The GHA webservice updates are failing

In trying to address 1, we discovered 2. This PR (in part) is related to 2, which remains an issue

Please take a look at the GHA webservice update log for more details

@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

2 is fixed in conda-forge/webservices-dispatch-action#41

From my POV, #1413 has left feedstocks with stuff in .github/workflows in an inconsistent state -- we shouldn't have files checked in that are also .gitignore'd. So either we should remove those files (assuming we can make things work without them), or un-ignore that folder.

Can you come up with a workflow with some bash commands that doesn't work? (except for the convoluted test that the webservices-dispatch-action is doing)

@h-vetinari
Copy link
Member

Can you come up with a workflow with some bash commands that doesn't work? (except for the convoluted test that the webservices-dispatch-action is doing)

This comes down to a matter of taste. You seem to think git add --force <otherwise_gitignored_file> is preferable to un-ignoring that folder. I think --force should not be necessary; i.e. files we need to check in should never be gitignored. But it's not a topic I feel strongly about, so 🤷

@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

Can you please tell me a workflow of a feedstock maintainer that needs to use git add --force? AFACIT, maintainers never need to use git add --force.

@jakirkham
Copy link
Member Author

Looking at the relevant templates

uses: conda-forge/automerge-action@{{ github.tooling_branch_name }}

uses: conda-forge/webservices-dispatch-action@{{ github.tooling_branch_name }}

It seems github.tooling_branch_name changes in conda-forge.yml could affect these files

Though IIUC github.tooling_branch_name was used to make a one time change from master to main. Idk if we would want to make that change again

If we don't think that change would occur, perhaps it makes sense to hard-code main and move these to feedstock_content?

@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

Same question to you @jakirkham.

@jakirkham
Copy link
Member Author

Hmm...think I've answered that above. Please let me know if you have any thoughts on the question posed at the end

@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

Since both of you don't like to give a reproducer, here's one I wrote for you.

sed -i.bak 's/tooling_branch_name: main/tooling_branch_name: main2/g' conda-forge.yml 
git add conda-forge.yml
conda smithy rerender
git commit

According to your analysis, is this supposed to fail?

@h-vetinari
Copy link
Member

h-vetinari commented Jan 4, 2024

According to your analysis, is this supposed to fail?

No, because the file is already checked in, so changes are tracked. But if we ever add a new template, or rename one of the existing ones, or some maintainer commits a deletion of .github1, then the content in .github would not be recommittable without --force. We can teach conda smithy rerender to git add --force it for the user in advance (in fact, 3.30.2 already does), but for example I always have to do git reset before committing2, which undoes that.

Since both of you don't like to give a reproducer

You're arguing against a strawman. My point is not about a reproducer, but about the cost-benefit we get from ignoring files we have checked in, and all the care necessary that no corner case will ever break due to this. But again: it's not important to me, you win.

Footnotes

  1. e.g. during some merge/rebase conflict involving a rerender where they delete all bot-generated content, with the expectation that a rerender will recreate it

  2. because there's an unrelated smithy bug that introduces spurious line-endings in .ci_support/README (on windows) if I don't

@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

Thanks you for sending examples. I appreciate it.

  1. But if we ever add a new template - The new template will be added by conda-smithy
  2. or rename one of the existing ones - The old template will be removed by conda-smithy
  3. or some maintainer commits a deletion of .github - Then they are committable without --force.

For eg following works

rm -rf .github
git add .github

because there's an unrelated smithy bug that introduces spurious line-endings in .ci_support/README (on windows) if I don't

Do you mind opening an issue? I don't want to break your workflow, so I want to support that.

@isuruf
Copy link
Member

isuruf commented Jan 4, 2024

There was a typo that I fixed in #1822 that should hopefully fix the GHA webservice updates

@h-vetinari
Copy link
Member

  • Ad 1./2. "The [...] template will be added/removed by conda-smithy" - yes, as long as we don't forget to add -f for any additional handling here.
  • Ad 3. "Then they are committable without --force.":

I may have not expressed myself well. I meant that newly rerendered content would not be readded after a deletion has been committed (unless using --force).

So here's a more complete example:

rm -rf .azure-pipelines .ci_support .circleci .github .scripts
git add .
git commit -m "WIP: temporarily delete bot-generated folders for merge conflict handling"
conda smithy rerender       # regenerate
git reset
git add .                   # no --force
git commit -m "rerendered"  # <- .github/workflows/automerge.yml does not get recommitted

Do you mind opening an issue? I don't want to break your workflow, so I want to support that.

#1823

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