Fedora Pungi mashing including module mashing. #1900

puiterwijk · 2017-10-16T16:19:16Z

No description provided.

ignatenkobrain · 2017-10-16T18:00:14Z

btw, does this PR mean that we will switch to pungi for mashing RPMs so we can proceed with rust packaging in fedora?

dustymabe · 2017-10-16T18:06:06Z

btw can we properly get my commits included in this patch set? here is the PR I opened: #1821. These show up in this patchset as a single foo commit.

dustymabe · 2017-10-16T18:23:38Z

we probably should not be embedding pungi configs in bodhi - can we delete the fedora-atomic.conf commit?

dustymabe · 2017-10-16T18:28:31Z

BTW we don't need to clone comps any more as pungi can now do that for us. The RFE was resolved in 4.1.18

bowlofeggs · 2017-10-16T18:56:20Z

@ignatenkobrain Yes, this patch will make Bodhi stop using mash and use Pungi to make RPM repos, OSTrees, and modules too. Exciting!

@dustymabe I'm actually planning to squash all of these commits down to one once we get this ready to merge, but I will attribute authorship to everyone involved in the commit message. Otherwise Bodhi's commit history will be really crazy (Bodhi has a contribution guideline about how commits should be clean and atomic.)

Conan-Kudo · 2017-10-16T19:09:15Z

Woohoo!

dustymabe · 2017-10-16T19:24:06Z

@dustymabe I'm actually planning to squash all of these commits down to one once we get this ready to merge, but I will attribute authorship to everyone involved in the commit message. Otherwise Bodhi's commit history will be really crazy (Bodhi has a contribution guideline about how commits should be clean and atomic.)

i'm pretty sure that's what I tried to do in #1821 - just look at this commit message and see how much information is included in it. I'd prefer not to squash everything into one massive commit.

bowlofeggs · 2017-10-16T19:34:03Z

On 10/16/2017 03:24 PM, Dusty Mabe wrote: i'm pretty sure that's what I tried to do in #1821 <#1821> - just look at this <630fb80> commit message and see how much information is included in it. I'd prefer not to squash everything into one massive commit.

I'm happy to make the commit message include the information we need, but I think the one massive commit is the way to go because I will need to cherry pick this feature to release it and because no commits will work on their own without the full set (thus, they are not atomic). The feature needs all of them to work, so it should be one commit.

bowlofeggs · 2017-10-16T19:40:23Z

I'll remove the fedora-atomic.conf.

dustymabe · 2017-10-16T19:48:02Z

I'm happy to make the commit message include the information we need, but I think the one massive commit is the way to go because I will need to cherry pick this feature to release it

Seems like this feature is so big we'd want a brand new release for it?

and because no commits will work on their own without the full set (thus, they are not atomic). The feature needs all of them to work, so it should be one commit.

Isn't that the point of a merge commit? I'm not saying we have to be super vigilant about breaking things into super small pieces, but concise commits with good messages about a specific diff is definitely more useful than a giant commit.

bowlofeggs · 2017-10-16T19:49:31Z

bodhi/server/consumers/masher.py

+                                        # Stderr should also go to pungi.global.log if it starts
+                                        stderr=subprocess.PIPE,
+                                        # We will never have additional input
+                                        stdin=DEVNULL)


I know /dev/null is special, but will there be a problem if multiple processess are given the same file handle to it? Should we make a call to open() for each process rather than opening one at the module-level?

bowlofeggs · 2017-10-16T19:52:30Z

bodhi/server/consumers/masher.py

+    def get_previous_compose(self):
+        """Function to return the previous composes' output."""
+        return None
+        return os.path.join(config.get('mash_stage_dir'), self.id)


Is the intention to kill this method, or was this return None just here for debugging?

This was for debugging.

bowlofeggs · 2017-10-16T19:54:26Z

bodhi/server/consumers/masher.py

+                self.log.exception("Repodata sanity check failed!")
+                raise
+
+            # make sure that pungi didn't symlink our packages


I was actually never really sure why Bodhi does this symlink check (it did it for mash too) - was there some problem in the past that symlinks caused? Does it happen often? Do you think we definitely need this?

It can create symlinks if you misconfigure pungi.
The problem is that symlinks become problematic with the syncing to /pub.
So this check is really there to make sure that Pungi wasn't misconfigured and got us a repo we can't sync out.

bowlofeggs · 2017-10-16T19:55:06Z

bodhi/server/consumers/masher.py

-        mash_path = os.path.join(self.path, self.id)
-        arch = os.listdir(mash_path)[0]
+        ## will need to fix mash_path here
+        mash_path = os.path.join(self.path, 'compose', 'Everything')


What do we need to fix about this?

Nothing anymore, that was fixed.

bowlofeggs · 2017-10-16T19:55:43Z

bodhi/server/consumers/masher.py

-                    msg=dict(tag=tag,
-                             ref=result.get('ref'),
-                             commitid=result.get('commitid')),
-                    force=True)


I can't wait to kill fedmsg-atomic-composer (which, humorously, isn't triggered via fedmsgs).

Well, fedmsg-atomic-composer does have a fedmsg consumer and can be ran through purely fedmsg.... :)

bowlofeggs · 2017-10-16T20:06:12Z

bodhi/server/consumers/masher.py

+    def copy_additional_pungi_files(self, pungi_conf_dir, template_env):
+        self.git_clone('https://pagure.io/fedora-comps.git',
+                       'master',
+                       os.path.join(pungi_conf_dir, 'comps'))


Dusty had suggested using Pungi to do this - I'm slightly inclined to make that a later commit/release just for reducing churn, but if you think that's easy we can do it now.

yeah that's what I was suggesting with #1900 (comment) - this is a large enough change that I would say we should just go ahead and leverage the new functionality in pungi

That's pretty easy.
When I looked at the documentation for it, I couldn't find a way to make it run "make", which in fedora-comps is required.
But after looking at the code, you can do that, so this entire call nd the "make" call can just be dropped.

bowlofeggs · 2017-10-16T20:06:55Z

bodhi/server/consumers/masher.py

+            self.log.error('out: %s', out)
+            self.log.error('Err: %s', err)
+            raise Exception('Unable to make comps')
+        shutil.copy('/etc/bodhi/variants-fedora.xml', pungi_conf_dir)


Will variants be specific to the release, or do all Fedora releases have the same variants document?

Let's make it just a template.

bowlofeggs

Thanks everyone, this patch looks pretty good so far! I'm going to start off by rebasing this onto #1903 once it's merged, and then I'll make a few minor tweaks of my own, and then I'll get to work writing tests and docs. This is a very exciting patch!

bowlofeggs · 2017-10-16T22:03:07Z

On 10/16/2017 03:48 PM, Dusty Mabe wrote: Seems like this feature is so big we'd want a brand new release for it?

It is getting its own release (well, almost. I'm going to sneak a backwards-incompatible CLI change as well, since we're naming this one Bodhi 3.0.0, but that one doesn't affect the server very much), which is why I will need to cherry pick it. There are many things on develop that I don't want to release with this patch, in case we need to back it out. This patch is going to go in with a FBR so I need it to be an easy downgrade.

Isn't that the point of a merge commit? I'm not saying we have to be super vigilant about breaking things into super small pieces, but concise commits with good messages about a specific diff is definitely more useful than a giant commit.

I also avoid merge commits, in order to keep a nice clean git history. My goal is to make it so that each commit on develop could be used as a release. This has a number of advantages: 1. I can easily cherry pick commits I want to bring into a release (I do this often). 2. Other bodhi deployments can also easily cherry pick the commits they want. 3. The git log is short and meaningful, without very many "oops fix typo" type commits (I did literally fix a typo today with a commit, but it was one that had already gotten into develop ☺). 4. Each feature and bug fix has exactly one commit, so it's easy to know you got it all when cherry picking. 5. Writing release notes is very painful when commits are not atomic. It's important to keep in mind that the commit author isn't necessarily the author of the patch, in general. While it's true that they are often the same in Bodhi, there are quite a few cases where they are not, and that's OK. There have been other commits in Bodhi that worked this way too. When more than one person collaborates on the same patch, the right thing to do is to credit them in the commit message, similar to how it would work if we authored a journal paper together. Bodhi also credits contributors to a release in the release notes, and so all the patch authors will also be credited there. The commit author is really just the person who created the commit. By the way, the requirement for commits to be atomic is documented in Bodhi's contribution guidelines: https://bodhi.fedoraproject.org/docs/developer_docs.html#contribution-guidelines

dustymabe · 2017-10-17T02:03:25Z

I also avoid merge commits, in order to keep a nice clean git history. My goal is to make it so that each commit on develop could be used as a release. This has a number of advantages:

I can easily cherry pick commits I want to bring into a release (I do this often).

Other bodhi deployments can also easily cherry pick the commits they want.

The git log is short and meaningful, without very many "oops fix typo" type commits (I did literally fix a typo today with a commit, but it was one that had already gotten into develop ☺).

I would never propose that we keep "oops fix typo" commits. Anyone that submits a PR should know how to properly rebase/squash commits and get rid of meaningless stuff like that.

Each feature and bug fix has exactly one commit, so it's easy to know you got it all when cherry picking.

A large feature in a single commit would probably lose a lot of details about "why". When "changing the way things are" like we are doing in this PR, we change where we look for things and we also delete code in some cases. Without the commit history we lose all of that context. I guess you can try to put it all in a single commit message, but I think most of it will get lost. A merge commit would keep the atomic requirement without losing the history. I don't really like merge commits either, but with the atomic requirement (which you have explained good reasoning for), they would solve the problem.

Writing release notes is very painful when commits are not atomic.

It's important to keep in mind that the commit author isn't necessarily the author of the patch, in general. While it's true that they are often the same in Bodhi, there are quite a few cases where they are not, and that's OK. There have been other commits in Bodhi that worked this way too. When more than one person collaborates on the same patch, the right thing to do is to credit them in the commit message, similar to how it would work if we authored a journal paper together. Bodhi also credits contributors to a release in the release notes, and so all the patch authors will also be credited there. The commit author is really just the person who created the commit.

I really don't care as much about having my name on commits. I hope I don't seem too vain :). I was more interested in keeping the history and meaningful commit messages that I worked to achieve.

By the way, the requirement for commits to be atomic is documented in Bodhi's contribution guidelines: https://bodhi.fedoraproject.org/docs/developer_docs.html#contribution-guidelines

yes I saw that, which is why I suggested a merge commit which would keep the meaningful history and also provide the atomicity required.

I'll stop beating this horse now. 🏇

bowlofeggs · 2017-10-17T02:07:08Z

I've rebased this PR locally, but weirdly GitHub is saying "Add more commits by pushing to the pungi-bodhi-modular branch on puiterwijk/bodhi", while git is saying "ERROR: Permission to puiterwijk/bodhi.git denied to bowlofeggs." I don't know what to make of that.

bowlofeggs · 2017-10-19T02:47:27Z

bodhi/server/consumers/masher.py

+                    # catches the same assumption breakage. This check here is extra for the
+                    # time when someone adds multitype updates and forgets to update this.
+                    raise ValueError('Builds of multiple types found in %s'
+                                     % title)


Thanks for the comment, and agreed on the no cover.

bowlofeggs

@puiterwijk I think I am good with the current patch set (though of course, I would want to squash it down before merging). I guess we'll wait and see what Jenkies thinks about the latest patch, and I'd like you to review the commits I made tonight too. If you and Jenkies both approve the PR I think we will be good to merge!

bowlofeggs · 2017-10-19T03:53:08Z

I did a brief survey of masher bugs, and here is a list of fixes lines I think we can include in the final commit message when we squash all this:

fixes #653
fixes #887
fixes #909
fixes #1182
fixes #1219
fixes #1330

For nice clickyness, here's those issues: #653, #887, #909, #1182, #1219, #1330.

bowlofeggs · 2017-10-19T04:11:04Z

Jenkies looks real backed up, but I ran the container tests on my laptop and they all passed here. So it'll probably pass… eventually…

bowlofeggs · 2017-10-19T13:43:58Z

OK this is now squashed down to one commit. @puiterwijk, do you approve of this PR as it stands?

bowlofeggs · 2017-10-19T13:47:18Z

I received an IRC approval to merge from @puiterwijk:

<fm-apps> github.pull_request.synchronize -- bowlofeggs synchronized pull request #1900 on fedora-infra/bodhi https://github.com/fedora-infra/bodhi/pull/1900
<fm-apps> github.issue.comment -- bowlofeggs commented on issue #1900 on fedora-infra/bodhi https://github.com/fedora-infra/bodhi/pull/1900#issuecomment-337911959
<bowlofeggs> puiterwijk: it is sqashed now - do you also approve the PR?
<bowlofeggs> if so, we can merge and i can start working on a beta
<puiterwijk> bowlofeggs: Yep. Make it so
<bowlofeggs> fantastic

This commit is a significant change in Bodhi, as it alters the repository layout that Bodhi generates when mashing. It also enables Bodhi to create repositories of a new content type, modules. Many other new features of Pungi are now available to us as well, such as rich dependencies. fixes fedora-infra#653 fixes fedora-infra#887 fixes fedora-infra#909 fixes fedora-infra#1182 fixes fedora-infra#1219 fixes fedora-infra#1330 This patch was written collectively by several authors: Kushal Das Dusty Mabe Adam Miller Patrick Uiterwijk Randy Barlow Signed-off-by: Randy Barlow <randy@electronsweatshop.com> Signed-off-by: Adam Miller <maxamillion@fedoraproject.org> Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org>

bowlofeggs · 2017-10-19T17:14:56Z

This has been cherry-picked to the 3.0 branch as 87267cc.

puiterwijk added the Composer Issues related to the composer label Oct 16, 2017

puiterwijk requested review from ralphbean and bowlofeggs October 16, 2017 16:19

bowlofeggs mentioned this pull request Oct 16, 2017

Pungi masher for modules #1697

Closed

fedora-infra deleted a comment from centos-ci Oct 16, 2017

bowlofeggs mentioned this pull request Oct 16, 2017

[nice-to-have] Create a Pungi Python API so Bodhi can drive Pungi via Pungi functions #1600

Closed

bowlofeggs reviewed Oct 16, 2017

View reviewed changes

bowlofeggs force-pushed the pungi-bodhi-modular branch from 9763c5e to f9c3178 Compare October 17, 2017 13:43

fedora-infra deleted a comment from centos-ci Oct 17, 2017

bowlofeggs force-pushed the pungi-bodhi-modular branch from f9c3178 to d454f14 Compare October 17, 2017 14:34

bowlofeggs reviewed Oct 19, 2017

View reviewed changes

bowlofeggs approved these changes Oct 19, 2017

View reviewed changes

fedora-infra deleted a comment from centos-ci Oct 19, 2017

bowlofeggs mentioned this pull request Oct 19, 2017

Bodhi needs to support alternative arches in mash config files #1685

Closed

fedora-infra deleted a comment from centos-ci Oct 19, 2017

This was referenced Oct 19, 2017

When updating comps, only update the file we need. #1799

Closed

Optionally wait for repo signatures before promoting #1830

Merged

fedora-infra deleted a comment from centos-ci Oct 19, 2017

bowlofeggs force-pushed the pungi-bodhi-modular branch from 1ab9419 to bbb7cb3 Compare October 19, 2017 13:43

bowlofeggs force-pushed the pungi-bodhi-modular branch from bbb7cb3 to f25e7ea Compare October 19, 2017 14:06

bowlofeggs force-pushed the pungi-bodhi-modular branch from f25e7ea to ffdda15 Compare October 19, 2017 14:09

fedora-infra deleted a comment from centos-ci Oct 19, 2017

bowlofeggs merged commit bb64d60 into fedora-infra:develop Oct 19, 2017

puiterwijk deleted the pungi-bodhi-modular branch October 19, 2017 15:33

Fedora Pungi mashing including module mashing. #1900

Fedora Pungi mashing including module mashing. #1900

Conversation

puiterwijk commented Oct 16, 2017

ignatenkobrain commented Oct 16, 2017

dustymabe commented Oct 16, 2017

dustymabe commented Oct 16, 2017

dustymabe commented Oct 16, 2017

bowlofeggs commented Oct 16, 2017

Conan-Kudo commented Oct 16, 2017

dustymabe commented Oct 16, 2017

bowlofeggs commented Oct 16, 2017 via email

bowlofeggs commented Oct 16, 2017

dustymabe commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs left a comment

Choose a reason for hiding this comment

bowlofeggs commented Oct 16, 2017 via email

dustymabe commented Oct 17, 2017

bowlofeggs commented Oct 17, 2017

Choose a reason for hiding this comment

bowlofeggs left a comment

Choose a reason for hiding this comment

bowlofeggs commented Oct 19, 2017 • edited Loading

bowlofeggs commented Oct 19, 2017

bowlofeggs commented Oct 19, 2017

bowlofeggs commented Oct 19, 2017

bowlofeggs commented Oct 19, 2017

bowlofeggs commented Oct 19, 2017 •

edited

Loading