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

Automatic pack namespace support #150

Closed

Conversation

stevegeek
Copy link
Contributor

@stevegeek stevegeek commented Feb 11, 2024

As per discussion in #144 would be nice to support automatic_pack_namespace.

I tried to make a stab at this but think I've hit my limits for now.

The changes here seem to produce the correct output with pks list-definitions but the test I added that uses the resolver ie failing... not sure why.

Also I don't know how best to implement the pack configuration parsing to actually parse out the configuration under a nested hash, ie automatic_pack_namespace is actually defined under metadata normally. At the moment I just added the automatic_pack_namespace boolean to the root of the package.yml so I could continue.

I also wasn't sure where to get the packs root path info from while determining the pack namespace name.

Finally I ran into an issue with wanting but go from a String value to a &String in a condition branch, but simply adding a & at the start of the expression resulted in errors about lifetimes of temporary values. So I added a let outside of the conditional but that just feels wrong!

Anyway the diff attached includes TODOs describing the above!

@stevegeek
Copy link
Contributor Author

@alexevanczuk i know you guys are probably very busy so no pressure on this, but just wanted to reiterate that I think I can't take this any further myself as reached my Rustfu limits. If you prefer I can close the PR and maybe it can be revisited in future by someone else!

@alexevanczuk
Copy link
Owner

Ah hey, totally up to you. Feel free to close if you don't want to leave dangling PRs open – I can always pull the closed branch to base new work off of.

I was actually just thinking of this as I was using automatic_pack_namespace in my org's codebase and it was bothering me that pks wasn't picking it up automatically and needed the packwerk.yml change. I'll probably get to this at some point.

@stevegeek
Copy link
Contributor Author

Ok thanks @alexevanczuk , happy to leave it open if you think you might pick it up in future!

@alexevanczuk
Copy link
Owner

Hey @stevegeek this is now implemented! #214

let me know if it works for you!

@stevegeek
Copy link
Contributor Author

@alexevanczuk Great thanks so much! Will try it out and let you know.

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