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

Clarify v2 stream specification #447

Closed
wants to merge 2 commits into from

Conversation

sgallagher
Copy link
Collaborator

Related to BZ 1799036

Several fields (notably those that provide N:S:V:C:A) were marked as being "optional" when that was really only because the packager should not be setting them.

Also separates the different spec sections with blank lines for readability.

Signed-off-by: Stephen Gallagher [email protected]

@sgallagher sgallagher added this to the 2.9.0 milestone Feb 11, 2020
@sgallagher sgallagher self-assigned this Feb 11, 2020
@goosemania
Copy link

👍 Thanks for the clarification, @sgallagher !

yaml_specs/modulemd_packager_v2.yaml Show resolved Hide resolved
yaml_specs/modulemd_stream_v2.yaml Show resolved Hide resolved
yaml_specs/modulemd_stream_v2.yaml Outdated Show resolved Hide resolved
yaml_specs/modulemd_stream_v2.yaml Outdated Show resolved Hide resolved
@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/fedora-modularity-libmodulemd-447
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

Copy link
Collaborator

@mmathesius mmathesius left a comment

Choose a reason for hiding this comment

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

This was obviously a lot of tedious work. I've got several tedious nit picks on some of the details.

yaml_specs/modulemd_stream_v2.yaml Outdated Show resolved Hide resolved
yaml_specs/modulemd_stream_v2.yaml Outdated Show resolved Hide resolved
yaml_specs/modulemd_stream_v2.yaml Show resolved Hide resolved
yaml_specs/modulemd_stream_v2.yaml Outdated Show resolved Hide resolved
tracker: http://www.example.com/

# profiles:
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are actually some existing things that I picked up on while re-reading this description...

  • Should something be explicitly noted about which profile names described below have special meaning and which are just arbitrary names? (And for the special names such as buildroot and srpm-buildroot refer to the component rpm flags below?)
  • Does default still have special meaning and get used as a default (as the description says it does)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to defer to @contyk here. Petr, how much of this is accurate?

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, "default" has no special meaning nowadays. "buildroot" and "srpm-buildroot" are the only snowflakes we have. I'm not sure what youy mean by the component rpm flags.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@contyk He's asking if we should include a pointer to data.components.rpms.<name>.[srpm]buildroot boolean attributes. I think the answer is yes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's what I meant.

yaml_specs/modulemd_stream_v2.yaml Outdated Show resolved Hide resolved
yaml_specs/modulemd_stream_v2.yaml Show resolved Hide resolved
ref: somecoolbranchname

# buildorder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# buildorder
# buildorder:

yaml_specs/modulemd_stream_v2.yaml Show resolved Hide resolved
@@ -3,15 +3,18 @@
# for a module stream. It is always a proper subset of a `document: modulemd`
# of the same `version`.
document: modulemd-packager
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more comment... Once the modulemd_stream_v2.yaml text is settled, it would be best to tediously replicate the applicable pieces here in modulemd_packager_v2.yaml as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it probably would. 😭

@sgallagher sgallagher force-pushed the spec_context branch 2 times, most recently from ee687aa to b3bfd15 Compare February 13, 2020 20:06
@sgallagher
Copy link
Collaborator Author

OK, I updated both the document: modulemd and document: modulemd-packager specifications to keep them in sync. I think I cleaned up the language of the buildroot options to make their purpose more clear.

In the process of reworking them to make them clearer, I had to update one of the tests which also revealed a minor bug in the output. We were emitting buildorder to the YAML as a 64-bit unsigned integer instead of as a 64-bit signed integer. This caused the test to fail when I set buildorder: -1 in one place (since it was comparing -1 to MAX_UINT64)

Another round of review would be helpful, please.

yaml_specs/modulemd_stream_v2.yaml Outdated Show resolved Hide resolved
yaml_specs/modulemd_stream_v2.yaml Outdated Show resolved Hide resolved
yaml_specs/modulemd_packager_v2.yaml Outdated Show resolved Hide resolved
yaml_specs/modulemd_packager_v2.yaml Outdated Show resolved Hide resolved
yaml_specs/modulemd_packager_v2.yaml Outdated Show resolved Hide resolved
Also updates the tests to validate the specification where changed.

Signed-off-by: Stephen Gallagher <[email protected]>
@sgallagher
Copy link
Collaborator Author

@mmathesius Changes made

Copy link
Collaborator

@mmathesius mmathesius left a comment

Choose a reason for hiding this comment

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

Thanks for fighting through this!

@sgallagher
Copy link
Collaborator Author

Merged in 80b253e

@sgallagher sgallagher closed this Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants