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

./code-generator/generate.sh relies on "advanced" / non cross platform features of bash #335

Open
banool opened this issue Sep 20, 2024 · 2 comments

Comments

@banool
Copy link
Contributor

banool commented Sep 20, 2024

I had to upgrade my bash version to 4.x.x to make the globstar option work.

To make adjust-cargo-toml.sh work (I'm on a Mac, this sed issue is classic) I had to do this:

sed -i '' '/\[features\]/,$d' ./kube-custom-resources-rs/Cargo.toml

I had to set K8S_OPENAPI_ENABLED_VERSION=1.31 for steps 3 and 4.

In create-custom-resources.sh I had to use mkdir -p "${version_directory}", not --parents.

At first I was using the wrong project_name. This one is mostly user error, I didn't read at first you that you have to update catalog.rs, it would've been obvious if I had. But it'd be nice if the tool errored out if the given project name is not present rather than silently "succeed".

Related to #336.

I can open a PR later.

@sebhoss
Copy link
Member

sebhoss commented Sep 21, 2024

yeah thanks for opening this & sorry for the trouble!

I have a unwritten todo list that contains at least some of those:

  • globstar/bash: I was aware that I wrote some bash specifics into those scripts and therefore declared bash as an interpreter. However, I'm totally fine re-writing those scripts to be POSIX shell compatible. I'm not entirely sure why I did not use find instead of that globstar feature but I think it should be doable. Likewise echo -n a few lines below that is not POSIX compatible and would need to change as well.
  • adjust-cargo-toml.sh: I want to use something like https://docs.rs/cargo-util-schemas/latest/cargo_util_schemas/manifest/struct.TomlManifest.html or maybe https://crates.io/crates/cargo_toml to adjust Cargo.toml with Rust code and get rid of the shell script.
  • K8S_OPENAPI_ENABLED_VERSION=...: I really hate this and it happened while working on Possible dependency version issue #289. According to https://docs.rs/k8s-openapi/latest/k8s_openapi/#crate-features this an expected failure, however I think we can just set the env variable in the shell scripts so that everyone gets the fix automatically.
  • mkdir --parents: Yep sorry - I kinda like to use long form flags in scripts but always forget that this is special 😓 - feel free to change that!
  • project_name: Agreed that an error message if nothing matched the given name would be nice!

I'll be away for a couple of days, therefore sorry in advance for slow replies..

sebhoss added a commit that referenced this issue Sep 26, 2024
relates to #335

Signed-off-by: Sebastian Hoß <[email protected]>
sebhoss added a commit that referenced this issue Sep 26, 2024
relates to #335

Signed-off-by: Sebastian Hoß <[email protected]>
@sebhoss
Copy link
Member

sebhoss commented Sep 26, 2024

I've pushed some changes in #339 that might work fine on Mac - would love to get some feedback whether that helped! I've used shellcheck to verify that each script is not using anything bash specific and re-wrote that one script with some rust code.

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

No branches or pull requests

2 participants