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

Add [email protected] #355

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Conversation

seh
Copy link
Contributor

@seh seh commented Jan 10, 2023

@fmeum
Copy link
Contributor

fmeum commented Jan 10, 2023

I triggered the pipeline, it fails with what looks like real issues.

Given that there is another rules_kustomize repo on GitHub, it may be clearer if you could add a prefix (e.g. org name) to the module name.

@seh seh force-pushed the impound-rules_kustomize-module branch from f105ebb to 4da696c Compare January 11, 2023 20:16
@seh
Copy link
Contributor Author

seh commented Jan 11, 2023

I triggered the pipeline, it fails with what looks like real issues.

Based on my fumbling but persistent effort chronicled in the "bzlmod" channel of the "Bazel" Slack workspace, I think that I fixed those problems.

Given that there is another rules_kustomize repo on GitHub, it may be clearer if you could add a prefix (e.g. org name) to the module name.

I do see a few other rules_kustomize GitHub repositories. None of them have MODULE.bazel files yet. @Wyverald had mentioned in Slack discussion that he thought that claiming rules_kustomize for my repository was acceptable. How should we proceed here? Should I prefix my module name with "bisontralis_?" Should I poll the authors of the other few repositories to learn whether any of them intend to submit their repositories to the BCR?

@seh seh force-pushed the impound-rules_kustomize-module branch from 4da696c to dec2379 Compare January 11, 2023 20:28
@seh
Copy link
Contributor Author

seh commented Jan 11, 2023

The CI failure in the test module is due to not running with the --experimental_use_sandboxfs flag enabled. I may be able to work around that in my own configuration of kustomize, just for this module, unless there's a way that I can request additional flags be used for how we run bazel here.

@fmeum
Copy link
Contributor

fmeum commented Jan 11, 2023

CI is failing with what looks like a real "non-Bzlmod" error.

Given that there aren't any rules about who can name what in which way, I don't see a problem with this just being called rules_kustomize. After all, distinguishing rules_kustomize from X_rules_kustomize isn't much harder than doing the same for X_rules_kustomize and Y_rules_kustomize.

@fmeum
Copy link
Contributor

fmeum commented Jan 11, 2023

@seh You should be able to specify build_flags and test_flags in the yaml. Does the ruleset itself require --experimental_sandbox_fs or is that just required for the tests? Given that this is one of the more arcane flags, getting this to work without it could make the module more generally useful.

@seh
Copy link
Contributor Author

seh commented Jan 11, 2023

Does the ruleset itself require --experimental_sandbox_fs or is that just required for the tests? Given that this is one of the more arcane flags, getting this to work without it could make the module more generally useful.

It's not strictly required, but it helps. I documented the considerations in the "Bazel's Sandbox and Load Restrictors" section of the repository documentation. If any of the information I offer there is now obsolete, since it's been over a year and a half since I wrote it, I'd like to update it.

I'll consider how best to accommodate the need in the CI workflow, and post an update either later today or tomorrow.

@seh seh force-pushed the impound-rules_kustomize-module branch from dec2379 to 6e0cc37 Compare January 12, 2023 01:53
@seh
Copy link
Contributor Author

seh commented Jan 12, 2023

I'll consider how best to accommodate the need in the CI workflow, and post an update either later today or tomorrow.

I decided to specify the "build_flags" and "test_flags" fields in the presubmit.yml file. If you find that sufficiently distasteful, I can instead change the "load restrictor" that kustomize uses, making it more liberal such that it will accept symlinked files.

@seh seh force-pushed the impound-rules_kustomize-module branch 2 times, most recently from ed63cfc to f912689 Compare January 12, 2023 16:41
@seh seh force-pushed the impound-rules_kustomize-module branch from f912689 to 4072c2f Compare January 23, 2023 14:52
@alexeagle
Copy link
Contributor

As much as I love to be a namespace purist and a "land grab" for shorter names is contentious (I'll probably be sad if someone takes "rules_js") I can also see a case that the better maintained or more widely used rulesets will more likely have someone to write the MODUlE.Bazel and any needed extensions.

@Wyverald
Copy link
Member

My take is that this one is fine as it's not super high profile. As Alex said, I'd be much more careful giving away names like "rules_js".

@seh
Copy link
Contributor Author

seh commented Jan 23, 2023

After much discussion with @fmeum in Slack (on execroot layout, repository mapping, runfiles, rlocationpath, and rlocation), borrowing from bazelbuild/bazel#17279 ahead of the next Bazel release, I think this patch will make it further through the CI workflow.

@seh
Copy link
Contributor Author

seh commented Jan 24, 2023

See bazelbuild/continuous-integration#1536 for the cause of the file permissions problems (lost executable bit).

@seh seh force-pushed the impound-rules_kustomize-module branch from 4072c2f to a7fc6f8 Compare January 24, 2023 14:11
@seh
Copy link
Contributor Author

seh commented Jan 24, 2023

In order to work around bazelbuild/continuous-integration#1536, I switched from using ZIP archive to a Tar archive of the GitHub-hosted repository.

I don't expect the Windows tests to pass yet, as I still have to figure out how to handle runfiles on Windows.

@seh seh force-pushed the impound-rules_kustomize-module branch from a7fc6f8 to e1bea96 Compare January 24, 2023 14:13
@alexeagle
Copy link
Contributor

@seh not a blocker for this PR - I don't remember in the many long threads on this, whether we discussed just shipping a source archive of the repo like most other rulesets. https://github.com/bazel-contrib/rules-template for example - then you wouldn't have to struggle with the archive format and it would probably simplify things for you.

@seh seh force-pushed the impound-rules_kustomize-module branch 4 times, most recently from 4f348d2 to 4192ad0 Compare January 27, 2023 21:46
Impound Bazel rules for the Kubernetes-related "kustomize" tool
defined in the "bisontrails/rules_kustomize" GitHub repository.
@seh seh force-pushed the impound-rules_kustomize-module branch from 4192ad0 to 3b2f56b Compare January 27, 2023 21:49
@fmeum fmeum merged commit 36532f8 into bazelbuild:main Jan 30, 2023
@seh seh deleted the impound-rules_kustomize-module branch January 30, 2023 14:07
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.

4 participants