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

Change build to use reusable workflows #169

Closed
wants to merge 2 commits into from

Conversation

webbnh
Copy link
Contributor

@webbnh webbnh commented Apr 3, 2024

Changes introduced with this PR

This PR reworks the build workflow

  • rename the workflow file to the conventional extension
  • switch to using reusable workflows for
    • lint and test
    • building the release
    • building the Python wheel

By contributing to this repository, I agree to the contribution guidelines.

@webbnh webbnh self-assigned this Apr 3, 2024
@webbnh webbnh marked this pull request as draft April 3, 2024 19:34
@webbnh webbnh force-pushed the reusable_workflows branch 3 times, most recently from 44f8a44 to dd7d416 Compare April 5, 2024 00:12
@webbnh webbnh marked this pull request as ready for review April 5, 2024 22:45
@webbnh
Copy link
Contributor Author

webbnh commented Apr 5, 2024

Now that arcalot/arcaflow-reusable-workflows#16 is merged, we're ready to try this.

- rename the workflow file to the conventional extension
- switch to using reusable workflows for
  - lint and test
  - building the release
  - building the Python wheel
@webbnh webbnh force-pushed the reusable_workflows branch from dd7d416 to 59c93b7 Compare April 5, 2024 23:02
@webbnh webbnh changed the title Testing reusable workflows Change build to use reusable workflows Apr 5, 2024
@webbnh
Copy link
Contributor Author

webbnh commented Apr 5, 2024

This PR is having the same problem as arcalot/arcaflow-docsgen#23 (comment) with the "release" check.

Also, it's having the same problem that arcalot/arcaflow-docsgen#22 had, with the organization variable values not being available in the called workflow. Unfortunately, unlike the Go release workflow which blunders on without it, the Python release workflow dies.

So, @jaredoconnell, unless someone has an objection, you should use your Admin override to merge it; and, if it doesn't run properly after that, we'll fix it then.

@jaredoconnell
Copy link
Contributor

Now that I think of it, I think we need to pass the org variables in from the calling workflow, and have them as input variables in the reusable workflows. I now remember Justin running into this with secrets.

@webbnh
Copy link
Contributor Author

webbnh commented Apr 9, 2024

I think we need to pass the org variables in from the calling workflow, and have them as input variables in the reusable workflows. I now remember Justin running into this with secrets.

Org variables, environment variables, inputs, and secrets are all separate, individual things with individual handling (and frustrations).

Org variables do work, once the workflow is merged in -- we already have evidence of that in go_lint_and_test.yaml. (Secrets, on the other hand -- except for the GITHUB_TOKEN 😛, which is special -- do have to be passed in.)

I suspect that passing the values of the org variables as inputs would get us past the problem of not being able to test prior to merging (and, it would better encapsulate/uncouple the reusable workflows), so it wouldn't be a bad thing. I had contemplated using that approach, but go_lint_and_test.yaml was already using the organization variable directly, and I didn't really want the two new workflows to be different, so I had them use the variables directly as well.

I s'pose I'll retool all three of them.

@webbnh webbnh marked this pull request as draft April 9, 2024 22:19
@webbnh
Copy link
Contributor Author

webbnh commented Apr 10, 2024

This PR is now back in draft awaiting arcalot/arcaflow-reusable-workflows#17.

@webbnh webbnh force-pushed the reusable_workflows branch from f9258b6 to ecdc502 Compare April 10, 2024 17:05
@webbnh webbnh mentioned this pull request Apr 12, 2024
@webbnh
Copy link
Contributor Author

webbnh commented Apr 12, 2024

This PR has been superseded by #171, in the hopes that, because that's pulling from a branch in the upstream repo, it will test cleanly.

@webbnh webbnh closed this Apr 12, 2024
@webbnh webbnh deleted the reusable_workflows branch April 12, 2024 18:07
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.

2 participants