Skip to content

DAOS-7821 gha: Update triggers for doxygen and release jobs.#7496

Merged
johannlombardi merged 12 commits intomasterfrom
gha-triggers
Dec 24, 2021
Merged

DAOS-7821 gha: Update triggers for doxygen and release jobs.#7496
johannlombardi merged 12 commits intomasterfrom
gha-triggers

Conversation

@ashleypittman
Copy link
Copy Markdown
Contributor

@ashleypittman ashleypittman commented Nov 29, 2021

Use the 'paths' setting in the GHA yaml file to decide when
to run jobs, rather than having them trigger for every PR
then exiting when there is nothing do to.

Signed-off-by: Ashley Pittman ashley.m.pittman@intel.com

Use the 'paths' setting in the GHA yaml file to decide when
to run jobs, rather than having them trigger for every PR
then exiting when there is nothing do to.

Run all installed headers under doxygen, we've got a much lower
error count now than when the trigger was added.

Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>
Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>
Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>
Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>
Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>
Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>
Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>
Copy link
Copy Markdown
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

on:
push:
paths:
- 'TAG'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@brianjmurrell this part is untested, however the syntax is the same as the doxygen file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've never used/tested paths:. As I read it, the syntax above seems to do what you are trying to do.

This could be tested by forking this repo into your personal github account, landing this PR and then landing a PR that adds a tag such as #7523. Just worth noting, that if you land this and it breaks, it won't be known until somebody wants to create a tag (i.e. an RC, etc.) in the future and will prevent it from being made. That will only be noticed by somebody paying attention to the version of the packages of the build and noticing that it has a git hash in it. Assuming somebody does notice, this will be a fire at that point that somebody will have to jump on fixing immediately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The paths option is new to me, but it makes sense for our use-cases here.

I've done a lot of testing in my personal fork of daos for this and other GHA actions, it's useful although somewhat time consuming to do. You're absolutely right that we won't know this is working until we go to use it, although it's a simple change and the doxygen change is easier to verity and seems to be working.

One downside with the current situation (other than additional complexity/load) is that whenever I push something to a fork it's run "create release" which always makes me look twice, and given the number of forks of daos out there presumably has the same reaction for other people as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The paths option is new to me, but it makes sense for our use-cases here.

Completely agreed.

I've done a lot of testing in my personal fork of daos for this and other GHA actions, it's useful although somewhat time consuming to do.

Agreed.

You're absolutely right that we won't know this is working until we go to use it,

If it's not tested in a fork, yes.

although it's a simple change and the doxygen change is easier to verity and seems to be working.

Indeed. My point is just that if it doesn't work, it's likely going to be a high-visibilty/high-priority fire to fix it as it will probably be blocking some kind of release-related activity.

One downside with the current situation (other than additional complexity/load) is that whenever I push something to a fork it's run "create release" which always makes me look twice, and given the number of forks of daos out there presumably has the same reaction for other people as well.

Also agreed. I think ultimately the change is a good change. I'm just warning on the potential future fire-fighting exercise that it's going to create if it doesn't work as expected for whatever reason. The fire-fight process is likely not going to remember this change was made (unless we land it and try it out for 2.0 while the change is still fresh in our minds) and going to require debugging to remember that the change was made and that it's the cause of the problem.

Ultimately, I don't have any strong objections to landing it, even untested. I'm just sensitive to the fact that if it does break, it's very likely going to land on me to drop whatever else I might be doing to debug and fix it. Maybe/hopefully this discussion will have now created a nice little place in my memory that I will immediately recall this change if it does break.

Copy link
Copy Markdown
Contributor

@brianjmurrell brianjmurrell Dec 6, 2021

Choose a reason for hiding this comment

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

FWIW, this all reminds me of CODEOWNERS. We (I) have looked at the GitHub docs for that many times in trying to get it to work and while everything I have tried seems like it should work according to the GitHub docs, it still does not work. That all despite knowing other projects have a functioning CODEOWNERS configuration.

Just to say, just because something is documented to work in some manner, it seems that reality can be different from documentation. At least in some cases/times.

Copy link
Copy Markdown
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

Copy link
Copy Markdown
Contributor

@brianjmurrell brianjmurrell left a comment

Choose a reason for hiding this comment

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

Looks fine with the caveat about the untested change to the "release" workflow and how it might impact making a future release.

on:
push:
paths:
- 'TAG'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've never used/tested paths:. As I read it, the syntax above seems to do what you are trying to do.

This could be tested by forking this repo into your personal github account, landing this PR and then landing a PR that adds a tag such as #7523. Just worth noting, that if you land this and it breaks, it won't be known until somebody wants to create a tag (i.e. an RC, etc.) in the future and will prevent it from being made. That will only be noticed by somebody paying attention to the version of the packages of the build and noticing that it has a git hash in it. Assuming somebody does notice, this will be a fire at that point that somebody will have to jump on fixing immediately.

@ashleypittman ashleypittman requested a review from cdurf1 December 7, 2021 09:27
Copy link
Copy Markdown
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@ashleypittman ashleypittman requested a review from a team December 13, 2021 17:21
Copy link
Copy Markdown
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

Copy link
Copy Markdown
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

Copy link
Copy Markdown
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Copy Markdown
Collaborator

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-7496/14/execution/node/1581/log

@johannlombardi johannlombardi merged commit 44a9d36 into master Dec 24, 2021
@johannlombardi johannlombardi deleted the gha-triggers branch December 24, 2021 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

7 participants