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

fix: better elixir docs and defaults #1207

Merged
merged 11 commits into from
Nov 15, 2024

Conversation

iloveitaly
Copy link
Contributor

Two main improvements:

  • docs: improve elixir build documentation
  • fix: better elixir defaults, missing packages

I don't think additional tests are needed here. Refactored to pattern off of the python provider a bit more closely.

Copy link
Contributor

github-actions bot commented Nov 4, 2024

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the stale The pull request is stale label Nov 4, 2024
@iloveitaly
Copy link
Contributor Author

Friendly reminder regarding this pull request! Please let me know if there are any further updates needed.

@coffee-cup coffee-cup added release/patch Author patch release and removed stale The pull request is stale labels Nov 4, 2024
Copy link
Contributor

@coffee-cup coffee-cup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Sorry its taken so long to get to and review.

I've updated the snapshot tests for elixir that was failing the build. Just one comment about the ecto.setup being removed, but besides that, it looks good to go.

@@ -51,9 +45,6 @@ impl Provider for ElixirProvider {
build_phase.add_cmd("mix assets.deploy".to_string());
}

if mix_exs_content.contains("postgrex") && mix_exs_content.contains("ecto") {
build_phase.add_cmd("mix ecto.setup");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, for me I didn't want this. Will add it back in.

The risk with running ecto.setup is it generally requires a DB connection, which is not available when the image is being built.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah thats no worries then. I'll merge as is but I wonder if we can detect if ecto setup is needed/will work

@coffee-cup coffee-cup merged commit cc3e10c into railwayapp:main Nov 15, 2024
100 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/patch Author patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants