#1464 Fix the name field in modulemd text for -devel builds
Merged 4 years ago by mikem. Opened 4 years ago by mikem.
mikem/fm-orchestrator devel-build-modulemd  into  master

@@ -276,7 +276,7 @@ 

                  u"module": {

                      u"module_build_service_id": self.module.id,

                      u"content_koji_tag": self.module.koji_tag,

-                     u"modulemd_str": self.module.modulemd,

+                     u"modulemd_str": self._get_fixed_mmd(),

                      u"name": ret["name"],

                      u"stream": self.module.stream,

                      u"version": self.module.version,
@@ -327,6 +327,16 @@ 

              u"type": u"rpm",

          }

  

+     def _get_fixed_mmd(self):
cqi commented 4 years ago

Method _finalize_mmd does same logic at line:

            mmd = mmd.copy(mmd.get_module_name() + "-devel")

It would be good to make this method accept argument mmd in order to be reused at these two places.

+         if self.devel:

+             mmd = self.module.mmd()

+             mmd = mmd.copy(mmd.get_module_name() + "-devel")

+             ret = mmd_to_str(mmd)

+         else:

+             ret = self.mmd

+ 

+         return ret

+ 

      def _get_arch_mmd_output(self, output_path, arch):

          """

          Returns the CG "output" dict for architecture specific modulemd file.
@@ -753,7 +763,7 @@ 

          mmd_path = os.path.join(prepdir, "modulemd.txt")

          log.info("Writing generic modulemd.yaml to %r" % mmd_path)

          with open(mmd_path, "w", encoding="utf-8") as mmd_f:

-             mmd_f.write(self.mmd)

+             mmd_f.write(self._get_fixed_mmd())

  

          mmd_path = os.path.join(prepdir, "modulemd.src.txt")

          self._download_source_modulemd(self.module.mmd(), mmd_path)

@@ -131,6 +131,11 @@ 

              # For devel, only check that the name has -devel suffix.

              assert ret["build"]["name"] == "nginx-devel"

              assert ret["build"]["extra"]["typeinfo"]["module"]["name"] == "nginx-devel"

+             new_mmd = module_build_service.utils.load_mmd(

+                 ret["build"]["extra"]["typeinfo"]["module"]["modulemd_str"])

+             assert new_mmd.get_module_name().endswith("-devel")

+             new_mmd = module_build_service.utils.load_mmd_file("%s/modulemd.txt" % file_dir)

+             assert new_mmd.get_module_name().endswith("-devel")

  

          # Ensure an anonymous Koji session works

          koji_session.krb_login.assert_not_called()

Currently for -devel builds, the name of the build does not match the name listed in the modulemd_str field and the modulemd.txt file. This change makes these values consistent.

Why are parentheses used here?

Why are parentheses used here?

Ah. They are left over from a previous edit. At one point the assert expression was too long for one line and I needed the parentheses to break it up. I'll remove them now since they are not needed.

@mikem, could you please fix the test that's failing?

could you please fix the test that's failing?

It wasn't failing for me locally before. I'll recheck

Ah, it's a py2 unicode thing

2 new commits added

  • fix unit test
  • drop unnecessary parentheses
4 years ago

pretty please pagure-ci rebuild

4 years ago

Method _finalize_mmd does same logic at line:

            mmd = mmd.copy(mmd.get_module_name() + "-devel")

It would be good to make this method accept argument mmd in order to be reused at these two places.

Method _finalize_mmd does same logic

I was tempted to do more overlap here, even perhaps work the logic into _finalize_mmd (when arch="noarch"), but the non-devel behavior isn't quite the same here. The duplication is a single, fairly straightforward line. Didn't seem worth the effort.

@mikem could you please squash your commits and rebase on the latest master?

The Jenkins job runs git merge with the --no-ff set, so if you aren't rebased on the latest master, the tests will fail.

rebased onto 8581b9d

4 years ago

pretty please pagure-ci rebuild

4 years ago

Build #485 failed (commit: 107df94).
Rebase or make new commits to rebuild.

The Jenkins job runs git merge with the --no-ff set, so if you aren't rebased on the latest master, the tests will fail.

I rebased, but I wanted to clarify what you meant here. It seems really absurd that having a PR against an older version of master will always cause CI to fail. My expectation would be that they would only fail if the merge could not be completed automatically (e.g. there is a merge conflict). The use of --no-ff should make no difference. That option just forces git to create the merge commit even when it could simply fast forward to accomplish the merge.

The Jenkins job runs git merge with the --no-ff set, so if you aren't rebased on the latest master, the tests will fail.

I rebased, but I wanted to clarify what you meant here. It seems really absurd that having a PR against an older version of master will always cause CI to fail. My expectation would be that they would only fail if the merge could not be completed automatically (e.g. there is a merge conflict). The use of --no-ff should make no difference. That option just forces git to create the merge commit even when it could simply fast forward to accomplish the merge.

I had misunderstood that option then. Perhaps it's caused by having the depth to 1:
https://pagure.io/fm-orchestrator/blob/master/f/.cico-pr.pipeline#_50

I'm only speculating and haven't tested it. This is the way it was setup for us by the CentOS CI folks. You can see the commit at 3675853.

Feel free to try tweaking the git settings.

Ah, the shallow clone for the main repo would be the problem. Git can't see far enough back on the master branch to merge. The shallow clone just doesn't have the needed commit.

git clone --single-branch --depth 1 https://pagure.io/fm-orchestrator.git"
cd fm-orchestrator && git remote add proposed \"${env.REPO}\""
cd fm-orchestrator && GIT_TRACE=1 GIT_CURL_VERBOSE=1 git fetch proposed"
cd fm-orchestrator && git checkout origin/master"
cd fm-orchestrator && git merge --no-ff \"proposed/${env.BRANCH}\" -m \'Merge PR\'"

I believe that using depth here serves little purpose. Normally this is done to make the clone operation a little faster and reduce the amount of data being transfered. However, in this case, the clone is quick either way (this isn't the kernel).

$ du -sh koji*
16M koji
11M koji.shallow

Furthermore, when the second remote is fetched (without a depth option), then we end up fetching more or less all the blobs from there that we omitted with --depth.

$ git clone https://pagure.io/fm-orchestrator.git
$ git clone --single-branch --depth 1  https://pagure.io/fm-orchestrator.git fm.shallow
$ du -sh fm*
67M fm-orchestrator
4.0M    fm.shallow
$ cd fm.shallow
$ git remote add dup https://pagure.io/fm-orchestrator.git
$ git fetch dup
$ cd ..
$ du -sh fm*
67M fm-orchestrator
68M fm.shallow

Ah, the shallow clone for the main repo would be the problem. Git can't see far enough back on the master branch to merge. The shallow clone just doesn't have the needed commit.
git clone --single-branch --depth 1 https://pagure.io/fm-orchestrator.git"
cd fm-orchestrator && git remote add proposed \"${env.REPO}\""
cd fm-orchestrator && GIT_TRACE=1 GIT_CURL_VERBOSE=1 git fetch proposed"
cd fm-orchestrator && git checkout origin/master"
cd fm-orchestrator && git merge --no-ff \"proposed/${env.BRANCH}\" -m \'Merge PR\'"

I believe that using depth here serves little purpose. Normally this is done to make the clone operation a little faster and reduce the amount of data being transfered. However, in this case, the clone is quick either way (this isn't the kernel).
$ du -sh koji*
16M koji
11M koji.shallow

Furthermore, when the second remote is fetched (without a depth option), then we end up fetching more or less all the blobs from there that we omitted with --depth.
$ git clone https://pagure.io/fm-orchestrator.git
$ git clone --single-branch --depth 1 https://pagure.io/fm-orchestrator.git fm.shallow
$ du -sh fm
67M fm-orchestrator
4.0M fm.shallow
$ cd fm.shallow
$ git remote add dup https://pagure.io/fm-orchestrator.git
$ git fetch dup
$ cd ..
$ du -sh fm

67M fm-orchestrator
68M fm.shallow

Could you file a separate PR that addresses this? Then we can merge it and you can then rebase your PR on that.

pretty please pagure-ci rebuild

4 years ago

pretty please pagure-ci rebuild

4 years ago

Tests seem to be passing now that #1469 and #1470 have been merged

Build #492 failed (commit: 107df94).
Rebase or make new commits to rebuild.

Build #493 failed (commit: 107df94).
Rebase or make new commits to rebuild.

Build #494 failed (commit: 107df94).
Rebase or make new commits to rebuild.

Build #497 failed (commit: 107df94).
Rebase or make new commits to rebuild.

The C3I tests won't pass because, at this revision, the tests have not been updated for the new deployment method. Sorry for the noise.

Commit 2b5deca fixes this pull-request

Pull-Request has been merged by mikem

4 years ago

Pull-Request has been merged by mikem

4 years ago