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

ci: add "exp" PR type #323

Merged
merged 5 commits into from
Mar 28, 2023
Merged

ci: add "exp" PR type #323

merged 5 commits into from
Mar 28, 2023

Conversation

pendo324
Copy link
Member

Issue #, if available:

Description of changes:

Testing done:

  • N/A

  • I've reviewed the guidance in CONTRIBUTING.md

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@pendo324 pendo324 requested a review from ningziwen March 28, 2023 18:26
@pendo324 pendo324 self-assigned this Mar 28, 2023
@pendo324 pendo324 changed the title add "expr" PR type chore: add "expr" PR type Mar 28, 2023
@pendo324 pendo324 force-pushed the add-expr-pr-type branch 2 times, most recently from 8606cf7 to 0e9ab2b Compare March 28, 2023 18:27
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
@pendo324 pendo324 marked this pull request as ready for review March 28, 2023 18:31
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Copy link
Member

@ningziwen ningziwen left a comment

Choose a reason for hiding this comment

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

Not language expert.. just want to raise the discussion to think more about the naming and messages.

.github/workflows/release-please.yaml Show resolved Hide resolved
.github/workflows/release-please.yaml Outdated Show resolved Hide resolved
@ningziwen
Copy link
Member

Should the pr title be "ci"?

Copy link
Member Author

@pendo324 pendo324 left a comment

Choose a reason for hiding this comment

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

Also wondering if we really need to run the full e2e tests for any .github/**.yaml changes. Should I also create a new ci workflow to only lint github workflow yaml, like how we do it in ci-docs?

.github/workflows/release-please.yaml Outdated Show resolved Hide resolved
.github/workflows/release-please.yaml Show resolved Hide resolved
@ningziwen
Copy link
Member

Also wondering if we really need to run the full e2e tests for any .github/**.yaml changes. Should I also create a new ci workflow to only lint github workflow yaml, like how we do it in ci-docs?

@pendo324 The e2e test itself can be added/changed in **.yaml so it is a bit different with .doc. Even specifying a subset of yamls, it can be risky.

Signed-off-by: Justin Alvarez <alvajus@amazon.com>
Signed-off-by: Justin Alvarez <alvajus@amazon.com>
@pendo324 pendo324 changed the title chore: add "expr" PR type chore: add "exp" PR type Mar 28, 2023
@pendo324 pendo324 changed the title chore: add "exp" PR type ci: add "exp" PR type Mar 28, 2023
@pendo324
Copy link
Member Author

Also wondering if we really need to run the full e2e tests for any .github/**.yaml changes. Should I also create a new ci workflow to only lint github workflow yaml, like how we do it in ci-docs?

@pendo324 The e2e test itself can be added/changed in **.yaml so it is a bit different with .doc. Even specifying a subset of yamls, it can be risky.

I added ci-github-yaml.yaml and it should only trigger for .github/**.yaml files. I agree that e.g. finch.yaml changes should run full e2e tests, but I'm not really sure if GitHub workflows/actions need to run the whole suite. I can remove it if you think it should be its own issue/PR though. WDYT?

@ningziwen
Copy link
Member

@pendo324 This will cause changing ci-github-yaml.yaml itself will not run e2e tests, which is risky and require more manual effort to review the changes.

Let's discuss in a separated issue/PR if you still want to propose this.

@pendo324
Copy link
Member Author

@pendo324 This will cause changing ci-github-yaml.yaml itself will not run e2e tests, which is risky and require more manual effort to review the changes.

Let's discuss in a separated issue/PR if you still want to propose this.

I don't necessarily think that's risky, but I see how changing the "actual" e2e test (ci.yaml) could be risky. I'll remove it and create a new issue to track this because I do think running all of the e2e tests for a simple yaml change is wasteful.

Signed-off-by: Justin Alvarez <alvajus@amazon.com>
@pendo324 pendo324 requested a review from ningziwen March 28, 2023 19:46
@pendo324
Copy link
Member Author

Skipping CI / e2e-tests since this change doesn't impact functionality (just a GitHub workflow YAML change).

@pendo324 pendo324 merged commit 983616f into runfinch:main Mar 28, 2023
pendo324 added a commit that referenced this pull request Mar 28, 2023
Issue #, if available:

*Description of changes:*
- Previously we added exp to release-please.yaml in #323, but
lint-pr-title has [its own configuration for custom
types](https://github.com/amannn/action-semantic-pull-request).
- Not doing this causes issue ([seen
here](https://github.com/runfinch/finch/actions/runs/4547098114/jobs/8016553677))
with PR titles using exp

*Testing done:*
  - N/A


- [x] I've reviewed the guidance in CONTRIBUTING.md


#### License Acceptance

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

Signed-off-by: Justin Alvarez <alvajus@amazon.com>
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.

None yet

2 participants