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

feat: add more convenience targets #121

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jjmaestro
Copy link
Contributor

@jjmaestro jjmaestro commented Dec 2, 2024

Add platform-aware :data and :control convenience targets at the top of the package repo so that we can use @<REPO>//<PACKAGE>:data directly. See changes to examples/debian_snapshot/BUILD:

diff --git a/examples/debian_snapshot/BUILD.bazel b/examples/debian_snapshot/BUILD.bazel
index 88b98ba..298c261 100644
--- a/examples/debian_snapshot/BUILD.bazel
+++ b/examples/debian_snapshot/BUILD.bazel
@@ -58,10 +58,7 @@ tar(
 
 cacerts(
     name = "cacerts",
-    package = select({
-        "@platforms//cpu:arm64": "@bullseye//ca-certificates/arm64:data",
-        "@platforms//cpu:x86_64": "@bullseye//ca-certificates/amd64:data",
-    }),
+    package = "@bullseye//ca-certificates:data",
 )
 
 oci_image(

Also, add :deps to complement :data.

Finally, with the changes in #115 and the previous commit, we can now remove a bunch of select from the examples and use the new target aliases.

NOTE: This PR adds to #115 some functionality that I had in #100 and we can close #100 completely.

@jjmaestro jjmaestro force-pushed the feat-add-more-convenience-targets branch 2 times, most recently from 776d3ca to 5c38a79 Compare December 3, 2024 12:18
@jjmaestro
Copy link
Contributor Author

@thesayyn sorry for the noise :( I'll back out the rebase and pull the last fix that changes the architecture map into a separate PR.

@jjmaestro
Copy link
Contributor Author

jjmaestro commented Dec 3, 2024

@thesayyn the noise comes from the stacked PRs, when I rebased this PR on top of #120, I didn't realize it would make it much harder to review 😞

I'm used to stacked PRs and rebases "just working" and GH is pretty terrible when it comes to this, I didn't realize it would dump all of the contents from your PR in here as changes :-/

Can you review #120 first? That PR is all straight-forward fixes, I hope it's easier to review.

Otherwise, you can focus on just this two commits::

  • b953eb7 - feat: add more convenience targets
  • 5c38a79 - chore: remove select() from the examples

@jjmaestro jjmaestro force-pushed the feat-add-more-convenience-targets branch from 5c38a79 to f164387 Compare December 3, 2024 18:42
@jjmaestro
Copy link
Contributor Author

OK, I managed to back out the rebase, this PR is now "clean" and separate from #120.

I'm really really sorry for all the noise, I'm still getting used to working with GH's limitations :(

@jjmaestro jjmaestro requested a review from thesayyn December 3, 2024 18:44
@jjmaestro jjmaestro force-pushed the feat-add-more-convenience-targets branch 3 times, most recently from affa9ec to 4a05c89 Compare December 5, 2024 17:39
@jjmaestro
Copy link
Contributor Author

@thesayyn I've further simplified the PR removing all of the "refactored" methods, so now it's just clear that the PR ditched the _PACKAGE_TEMPLATE in favor of reusing package_template.

Let me know if the PR is to your liking, I'll be happy to make more changes if needed. Thanks.

@jjmaestro jjmaestro force-pushed the feat-add-more-convenience-targets branch from 4a05c89 to 3327243 Compare December 5, 2024 20:48
@jjmaestro jjmaestro requested a review from thesayyn December 5, 2024 20:50
@jjmaestro jjmaestro force-pushed the feat-add-more-convenience-targets branch 3 times, most recently from 99e0b8f to b198392 Compare December 10, 2024 20:48
@jjmaestro
Copy link
Contributor Author

@thesayyn I'm going to rebase the branch on main to remove the "Conflicting files" message from the threads but you now have the "clean Compares" to check.

Learning to work around GH limitations... 😅

Mirror the "inner" (per-arch) :data and :control convenience targets to
platform-aware targets at the top of the package repo so that we can use
`@<REPO>//<PACKAGE>:data` directly (see changes to
examples/debian_snapshot/BUILD).
With the changes in GoogleContainerTools#115 and the previous commit, we can now remove a
bunch of select from the examples and use the new target aliases.
@jjmaestro jjmaestro force-pushed the feat-add-more-convenience-targets branch from b198392 to 860dd9c Compare December 10, 2024 20:53
@jjmaestro
Copy link
Contributor Author

jjmaestro commented Dec 10, 2024

@thesayyn and, again, after I did the two pushes before the rebase, somehow GH is not showing the "Compare" for the two of them, only for the last one (the (...) most recently from (...))!??

This makes it impossible to see the changes I did to address your comments... this is absurd, I really don't know how GH can be failing at this stuff o.0.

Here's my "manual compare link" between the changes: Compare 3327243..b198392.

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.

2 participants