Skip to content

Conversation

bobleesj
Copy link
Contributor

No description provided.

@bobleesj bobleesj changed the title Implement central GitHub CI workflow Implement central GitHub CI workflow (do not merge - testing for now!) Sep 12, 2024
@bobleesj bobleesj changed the title Implement central GitHub CI workflow (do not merge - testing for now!) Implement central GitHub CI workflow Sep 13, 2024
@bobleesj
Copy link
Contributor Author

bobleesj commented Sep 13, 2024

@sbillinge Could you please merge?

The news item CI will be available once it is merged due to the keyword of pull_request_target in the CI news.

To use the pull_request_target event in GitHub Actions, the workflow configuration must already be present in the base branch at the time the pull request is opened or updated.

This behavior was reproduced in diffpy.snmf - diffpy/diffpy.snmf#79)

@bobleesj bobleesj marked this pull request as ready for review September 13, 2024 14:48
Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

This looks great, super exciting. I wonder if we want to finish testing, merge into cookiecutter and then recut everything so it is all the same and takes less review.

For example, I see here that @Tieqiong's indents for his lists are further to the left than we have been using conventionally. This is fine of course, but then breaks a standard we have been using, so before we roll this out to all the repo's let's at least discuss it.

Also I initiated a discussion about whether we want to make branches that capture different versions as seems to be a standard for other workflows, so it is @v1, @v2 instead of @main. From a practical perspective, its no different because we will sync main with v# branch, but since we want to create a standard that we use across all projects, let's work to decide what the standard is. The nice thing about recookiecutting after we have done this is that all the changes since the last cookiecut are incorporated in all the projects.

@bobleesj
Copy link
Contributor Author

bobleesj commented Sep 14, 2024

This looks great, super exciting. I wonder if we want to finish testing, merge into cookiecutter and then recut everything so it is all the same and takes less review.

Yes, I agree that we shall update the .yml files while recutting.

For example, I see here that @Tieqiong's indents for his lists are further to the left than we have been using conventionally. This is fine of course, but then breaks a standard we have been using, so before we roll this out to all the repo's let's at least discuss it.

If you ask me, I suggest that we stick with how Github does it (our previous way) since we may also need to copy a portion of their boilerplate .yml files in the future as well: https://docs.github.com/en/actions/use-cases-and-examples/building-and-testing/building-and-testing-python

Also I initiated a discussion about whether we want to make branches that capture different versions as seems to be a standard for other workflows, so it is @v1, @v2 instead of @main

Right, @v1, @v2, seem like a great solution. From a maintainer's perspective, seeing@v3 is much easier to check the current status of the repo than seeing @main

Update: seeing Tieqiong's comment below, using @main makes sense since the goal is not to update each repo.

@Tieqiong
Copy link
Contributor

Tieqiong commented Sep 14, 2024

For the indentation I think our previous way is better. It was probably caused by the yaml package used in update_workflow.py which follows YAML 1.1 parser not the GitHub workflow file convention. I can modify the update_workflow.py file to get those spaces back. See scikit-package/release-scripts#25

For the versioning, see the discussion in scikit-package/scikit-package#126. I might be a bit unclear but I meant we should still have @main in our dummy yaml files across the repos (because we don't want to go to all of the repos and change v2 to v3 manually when we update to v3 for example), but we can make version branches in release-script in case we want to reuse any of the old workflows.

@sbillinge
Copy link
Contributor

For the indentation I think our previous way is better. It was probably caused by the yaml package used in update_workflow.py which follows YAML 1.1 parser not the GitHub workflow file convention. I can modify the update_workflow.py file to get those spaces back. See Billingegroup/release-scripts#25

For the versioning, see the discussion in Billingegroup/cookiecutter#126. I might be a bit unclear but I meant we should still have @main in our dummy yaml files across the repos (because we don't want to go to all of the repos and change v2 to v3 manually when we update to v3 for example), but we can make version branches in release-script in case we want to reuse any of the old workflows.

We wouldn't have to update the repos, just keep main and v0 synced. We only break into v1 if we want v0 and v1 to both be available to different repos. I am still not convinced that we should go with main tbh, though I get the argument

@Tieqiong
Copy link
Contributor

I think I get the idea now. So instead of having versioning as a progressive changes on the same set of workflows, we use version number to map repos with the corresponding set of workflows.

For example we can use v0 for everything we have now, and make for example v1 for another set of different workflows (potentially with the same file names) for any future repos that need different workflows, so that we can make release-script truly the central repo for any kind of workflows set.

This indeed would be very nice to have.

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

@bobleesj please can you check? Should this be closed because of @Tieqiong 's work? Or is this still relevant?

@bobleesj
Copy link
Contributor Author

Yes, closing this as a part of #101

@bobleesj bobleesj closed this Sep 17, 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.

3 participants