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

Remove manpage generation from Tito build #11183

Merged

Conversation

stevekuznetsov
Copy link
Contributor

If a Tito build is done from a commit checked into origin/master,
the man pages will by definition be up-to-date, as verifying that
is a prerequisite for pull request acceptance.

Signed-off-by: Steve Kuznetsov [email protected]

If a Tito build is done from a commit checked into origin/master,
the man pages will by definition be up-to-date, as verifying that
is a prerequisite for pull request acceptance.

Signed-off-by: Steve Kuznetsov <[email protected]>
@sdodson
Copy link
Member

sdodson commented Oct 3, 2016

I think this is in there for downstream projects like OSE. Right @tdawson?

@tdawson
Copy link
Member

tdawson commented Oct 3, 2016

That was put in there because of #9407
If things have changed now and this is no longer needed, I am fine with removing it.

@stevekuznetsov
Copy link
Contributor Author

Hmm, I think in Origin we will want to not generate them since it's "extra" work - could we make it optional? Or do you think we should have a command to generate them like the one in oc for completions?

@stevekuznetsov
Copy link
Contributor Author

@sdodson @tdawson hold on, don't we have the same constraint on the downstream project? Nothing can merge into master without verifying that the manpages checked in match the code. Am I missing something?

@sdodson
Copy link
Member

sdodson commented Oct 4, 2016

I should mention the man page content downstream is all different because of the changes to the content, 'OSE' or 'OCP' versus 'Origin' in pretty much every man page. In the past we've not blocked merges on the test but we probably should, building the manpages in the RPM build process was our safety net against those problems.

What's your concern with leaving this step in the rpm build process? Time?

@stevekuznetsov
Copy link
Contributor Author

man page content downstream is all different

This is understood, but surely when you run the manpage generation in the merge queue for that project they are generated correctly, right?

What's your concern with leaving this step in the rpm build process? Time?

Yes, once my tool is put into use for our CI system, this tito step will be part of the critical path for testing a change. We need to make it as lean as possible, this means removing excess work (like this PR) and determining how we can re-use work as well (like #11184)

@stevekuznetsov
Copy link
Contributor Author

In the past we've not blocked merges on the test

This is unsettling to me.

@tdawson
Copy link
Member

tdawson commented Oct 5, 2016

So, I've been pretending I know what's going on with the "merge queue", and at this point I'm going to stop doing that and ask questions.

When exactly do these man pages get generated if not in the rpm?

If I do a merge does this do more than just merge? I know it tests. But is it also twiddling bits behind my back?

For all of the origin 1.3 / ose 3.3 the test queue, and thus the merge queue was always broken. We didn't do a single merge the whole time, it was all manual testing and then a manual merge (push the big green button)

For origin 1.4 / ose 3.4 we've fixed the testing, and so we are now doing merge's.

@stevekuznetsov
Copy link
Contributor Author

Tagging a PR with the merge tag will fire off a Jenkins job that spawns EC2 instances in which a number of tasks are done, one of which is similar to the make verify task in Origin. This will generate the man pages and flag a PR for which the man pages (checked in to the repo in docs/man/) do not match those that are generated from the currently checked in source code. If all of the contributions to the branch from which an RPM is being built are done through the queue, you should be able to simply copy in docs/man.

@stevekuznetsov
Copy link
Contributor Author

Speaking of which, let's be careful when we throw around that tag :)
@openshift-bot please calm down, no merge necessary

@stevekuznetsov
Copy link
Contributor Author

Rosie please no!

@tdawson
Copy link
Member

tdawson commented Oct 5, 2016

Sorry about putting that in the comments. I get to typing and totally forgot what happens when you type that.

Thanks for the explanation. I didn't know the merge did that. I'll be doubly sure to make sure the 1.4 / 3.4 get a proper merge.

As for the original request, I just checked and it looks like for 1.4 / 3.4 the man pages are being updated properly during "the merge" So I have no problem removing this step.

@stevekuznetsov
Copy link
Contributor Author

@tdawson since the specfile is grabbing the documentation from docs/man, I think this is the only change we need to make. Merge on your word :)

@tdawson
Copy link
Member

tdawson commented Oct 5, 2016

You are correct, that will do it. Give it a merge.

@stevekuznetsov
Copy link
Contributor Author

[merge] this time I mean it @openshift-bot

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 3f2bf48

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9665/)

@stevekuznetsov
Copy link
Contributor Author

@stevekuznetsov
Copy link
Contributor Author

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 3f2bf48

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 6, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9704/) (Image: devenv-rhel7_5140)

@openshift-bot openshift-bot merged commit 1f1d8bf into openshift:master Oct 6, 2016
@stevekuznetsov stevekuznetsov deleted the skuznets/specfile-edit branch January 31, 2017 14:21
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.

4 participants