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

add clean kwarg to doc builds #58156

Merged
merged 2 commits into from
Oct 5, 2020
Merged

add clean kwarg to doc builds #58156

merged 2 commits into from
Oct 5, 2020

Conversation

Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Aug 7, 2020

When building the docs locally for development I dont always want to clean out the directory, to ensure consecutive builds are quicker.

@Ch3LL Ch3LL requested a review from a team as a code owner August 7, 2020 21:10
@ghost ghost requested review from dwoz and removed request for a team August 7, 2020 21:10
@Ch3LL Ch3LL requested a review from s0undt3ch August 7, 2020 21:10
s0undt3ch
s0undt3ch previously approved these changes Aug 7, 2020
Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

We also have to update the pipeline libs as we merge this.
@bryceml should we start running doc builds in a container on GH Actions?

@bryceml
Copy link
Contributor

bryceml commented Aug 17, 2020

We also have to update the pipeline libs as we merge this.
@bryceml should we start running doc builds in a container on GH Actions?

I'd be cool with that if the core team is. The only downside is you can't just go to jenkins and see "all green" in one place. We're supposed to get labhub going this release cycle anyway though, so we could do it on gitlab. Things would be more all in once place if we did that since the tiamat pr builds will be in gitlab.

If it's all done in a container, it's trivial to move between github actions and gitlab though.

@Ch3LL
Copy link
Contributor Author

Ch3LL commented Sep 15, 2020

im okay with whatever is easiest for you to manage @bryceml and easy for us to see at the same time. Is this decision blocking from having this approved to be merged?

@bryceml
Copy link
Contributor

bryceml commented Sep 15, 2020

no, that's not blocking this, this looks good to me.

bryceml
bryceml previously approved these changes Sep 15, 2020
@dwoz dwoz added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Sep 22, 2020
@bryceml
Copy link
Contributor

bryceml commented Oct 1, 2020

This needs merged directly after saltstack/salt-jenkins-pipeline-libs#35

do we want to time that? is there a cleaner path than interrupting all old prs and needing to update branch everywhere?

can we add the ability to call it both ways for a while until all prs get caught up?

@Ch3LL thoughts?

Another option is to leave this pr as is and let the pipeline figure out which one to run.

@bryceml bryceml removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Oct 1, 2020
@bryceml
Copy link
Contributor

bryceml commented Oct 1, 2020

something like this maybe? saltstack/salt-jenkins-pipeline-libs#42

@bryceml
Copy link
Contributor

bryceml commented Oct 1, 2020

we could also bump the library version for this, that's probably the cleanest way actually.

@Ch3LL
Copy link
Contributor Author

Ch3LL commented Oct 1, 2020

We can wait to merge this if its going to cause people to rebase. This is just a nice to have, but yeah whichever approach works for me.

@bryceml bryceml added Aluminium Release Post Mg and Pre Si Magnesium Mg release after Na prior to Al labels Oct 2, 2020
@bryceml
Copy link
Contributor

bryceml commented Oct 5, 2020

There should be no issue getting this into Magnesium, but it would be fine going into Aluminum too.

@dwoz dwoz merged commit 15f7853 into saltstack:master Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si has-failing-test Magnesium Mg release after Na prior to Al
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants