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

Deployment and repo updates #15

Merged
merged 4 commits into from
May 6, 2024
Merged

Deployment and repo updates #15

merged 4 commits into from
May 6, 2024

Conversation

GrantBirki
Copy link
Contributor

@GrantBirki GrantBirki commented May 6, 2024

This pull request layers all my changes that I made in my own fork into this project.

Most of the changes here is related to me vendoring shards into the lib/ directory. This helps to maintain repeatable builds in CI. Also, I ran crystal docs. Between these two things, it made the PR diff massive so for that I do apologize

This PR also adds support for deployments.cr as well 🚀

@GrantBirki
Copy link
Contributor Author

@watzon here are all my changes that I layered in. I'm using all these exact changes over in a new project and they are working as expected. That project where this is all working can be found here.

@watzon
Copy link
Collaborator

watzon commented May 6, 2024

It's generally discouraged to vendor libraries like that. Have you considered tagging a specific commit for each dependency in shard.yml instead?

@GrantBirki
Copy link
Contributor Author

It's generally discouraged to vendor libraries like that. Have you considered tagging a specific commit for each dependency in shard.yml instead?

I did briefly look into doing that. The issue that I ran into though (this happened to the OSS community many times in the past.. cough nodejs... npm.. cough) is where an upstream repo is fully deleted. So for example, we have runtime dependency on halite. If the icyleaf/halite repo is suddenly deleted, we have no realistic way of building this project unless we do one of the following:

  • Maintain a fork of that project
  • Vendor the entire project by committing the lib/ directory

This is a highly opinionated dependency management approach and its something a lot of folks in the Ruby community follow religiously. Its also something that I try my best to follow as well because of the whole who owns your availability... you... concept.

I have also been chatting quite a bit with some of the maintainers of crystal about the best way to vendor shards and they did suggest doing exactly this as the "best way" to vendor shards in crystal's current state.

That being said, if you don't think vendoring shards in the lib/ dir is something you want to do, I can certainly remove it. I think it does add a fair bit of value here though as its protects the project from upstream issues.

Let me know what you think!

@watzon
Copy link
Collaborator

watzon commented May 6, 2024

Gotcha. I'm not wholly opposed to it, but it definitely makes commits like this harder to parse. It might be worth looking into removing halite as a dependency entirely, since it's actually quite a bit slower than the built in HTTP client. When I added it I did so just because I liked halite's API, but I ended up having issues with it in another project so I haven't been using it anymore.

Another option outside of vendoring, and something I tend to do myself more often would be to just fork the projects into this org and reference that fork in shard.yml. For now I'll go ahead and merge this though, and that's something we can look at in the future.

@watzon watzon merged commit 4f85a43 into master May 6, 2024
3 checks passed
@watzon watzon deleted the deployment-and-repo-updates branch May 6, 2024 22:36
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