-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add publish workflow to Github Actions #292
Conversation
WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue. |
- master | ||
tags: | ||
- '*' | ||
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work well. It raises an error if on
is empty: No event triggers defined in 'on'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it sufficient to just let this happen nightly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the workflow or the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion here and can change it to only run nightly. Any objections @pulp/continuous-integration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no objections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it useful being able to see the built docs right away. What is the concern about the frequency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main reasons were
- It doesn't feel like a good enough reason by itself to maintain a totally separate workflow.
- If we really do want to run it any time a branch gets pushed, it doesn't need to be a separate workflow. The normal "ci" workflow already runs on every push, we could just add a condition for the case when it was triggered by a push to go and publish the docs at the end.
- Or, if there is a bunch of stuff that falls into that category (do publish docs, don't lint commits or lint code), we could make a job called "push.yml" and keep the same naming convention we've got going on. If we're tweaking more than one thing it would feel more justified.
- But if just running it nightly is "good enough", that would be the lowest maintenance or CI minute burden of any of the options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed an issue here https://pulp.plan.io/issues/7910 to continue the conversation. I'm going to merge this so we can start on the pulp_file 1.4.0 release and then revisit later.
cf0217a
to
6641f2a
Compare
@daviddavis, did you intentially move |
@dralley I did. I meant for |
That's fine, I was just checking since I think it was one of the things that was suggested to be moved to .ci/ previously. |
8ca31db
to
7751415
Compare
eval "$(ssh-agent -s)" #start the ssh agent | ||
ssh-add ~/.ssh/pulp-infra | ||
|
||
if [[ $GITHUB_REF = "refs/heads"* ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be more comfortable if this was a script argument rather than implicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would make sense if this was a generic script but I meant for this to be specific to the Github Actions environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, it just feels hacky. No strong objection to continuing though
d266cf7
to
4ea437a
Compare
a1409dd
to
e6c85c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Shouldn't this reference the issue? (https://pulp.plan.io/issues/7880) |
fixes #7880
e6c85c2
to
d91677b
Compare
[noissue]