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

refactor pkg_deb #446

Merged
merged 18 commits into from
Nov 8, 2021
Merged

refactor pkg_deb #446

merged 18 commits into from
Nov 8, 2021

Conversation

aiuto
Copy link
Collaborator

@aiuto aiuto commented Oct 24, 2021

Refactor debian building so it can be more easily owned by others.

blocked by #443

cc: @alexeagle

@aiuto aiuto requested a review from nacl as a code owner October 24, 2021 02:15
CODEOWNERS Show resolved Hide resolved
@aiuto
Copy link
Collaborator Author

aiuto commented Nov 4, 2021

This is ready again, now that the big reorganization PR is merged.

@nacl
Copy link
Collaborator

nacl commented Nov 4, 2021

We don't happen to have tests that exercise the distro tarball, would we? A simple way to do this would be something that would build the distro, extract it somewhere, plant the tests inside the extracted location, and then run the tests.

Copy link
Collaborator

@nacl nacl left a comment

Choose a reason for hiding this comment

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

I think we need to confirm the policy on CODEOWNERs first so we prevent shooting ourselves in the foot.

LGTM otherwise, will approve once we come to a consensus on it and naming.

CODEOWNERS Show resolved Hide resolved
distro/BUILD Show resolved Hide resolved
distro/testdata/BUILD.tpl Show resolved Hide resolved
pkg/README.md Outdated Show resolved Hide resolved
@aiuto
Copy link
Collaborator Author

aiuto commented Nov 4, 2021

We don't happen to have tests that exercise the distro tarball, would we? A simple way to do this would be something that would build the distro, extract it somewhere, plant the tests inside the extracted location, and then run the tests.

Yes. The tests under distro do do that. They make sure we can build some things when using the tarball, and under a different repository name.
We could probably add finer grained test in the future.

@aiuto
Copy link
Collaborator Author

aiuto commented Nov 7, 2021

I think we need to confirm the policy on CODEOWNERs first so we prevent shooting ourselves in the foot.

What particular foot shooting are you thinking of?
The current situation is that you and I are the only codeowners, and with the current branch protection policy, that means a PR from either of us is blocked on the other.

Adding extra owners will allow a domain expert to review and approve a change. If it is strictly to RPM or DEB packaging (or maybe even tar & zip) I welcome the ability to have someone else be on the hook to review things.
That's what this will do.

@nacl
Copy link
Collaborator

nacl commented Nov 7, 2021

What particular foot shooting are you thinking of? The current situation is that you and I are the only codeowners, and with the current branch protection policy, that means a PR from either of us is blocked on the other.

Adding extra owners will allow a domain expert to review and approve a change. If it is strictly to RPM or DEB packaging (or maybe even tar & zip) I welcome the ability to have someone else be on the hook to review things. That's what this will do.

I was mostly concerned regarding exclusivity to owners -- that if I were the only owner of the RPM code and I were unavailable, then a change couldn't be approved without editing the CODEOWNERs files.

Reading more closely, I see that this is more of an overlay and not really a problem. Will approve momentarily.

Copy link
Collaborator

@nacl nacl left a comment

Choose a reason for hiding this comment

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

My concerns are addressed in comments and code updates. LGTM.

@aiuto aiuto merged commit b51e0b4 into bazelbuild:main Nov 8, 2021
@aiuto aiuto deleted the deb_own branch November 8, 2021 04:16
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.

3 participants