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

chore: Prepare for development #12

Merged
merged 19 commits into from
Feb 20, 2025
Merged

chore: Prepare for development #12

merged 19 commits into from
Feb 20, 2025

Conversation

ignaciogoldchluk-yolo
Copy link

@ignaciogoldchluk-yolo ignaciogoldchluk-yolo commented Feb 17, 2025

  • Github Actions workflow to test the package
  • Add .tool-versions
  • Fix warnings on newest Elixir
  • Delete old Docker/compose and Travis config files
  • Delete coveralls (we can add back again it later)
  • Add script to fetch deps + start server + test
  • Remove unused library :httpoison
  • Remove uuid and vendor Ecto.UUID.generate/0 instead
  • Replace mock with mox, or delete if the dependency is not required
  • Remove mix_test_watch
  • Bump ex_doc
  • Bump stream_data
  • Replace poison with jason
  • Add Github Actions "release" workflow.
    • Not entirely sure about this one because the name gremlex is already taken. We would have to create a new package, or if we do not plan on publishing it then we will always have to reference via git
  • Update README.md
  • Fix flaky tests

There is only one caveat/issue. :socket library is unmaintained, and it throws type errors. The library could be replaced by https://github.com/elixir-mint/mint_web_socket but it might require some additional refactoring.

{:confex, "~> 3.2.3"},
{:uuid, "~> 1.1"},
{:jason, "~> 1.4"},
{:confex, "~> 3.0"},

Choose a reason for hiding this comment

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

Maybe remove confex and use System or Application module?

Choose a reason for hiding this comment

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

I believe that would be a breaking change? confex is for users to set variables as {:system, "GREMLEX_HOST", "127.0.0.1"} instead of System.get_env("GREMLEX_HOST", "127.0.0.1"). The thing is confex supports also file config, casting to integer, atom, etc.

graph:
image: tinkerpop/gremlin-server:3.7
ports:
- 8182:8182

Choose a reason for hiding this comment

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

Suggested change
- 8182:8182
- 8183:8182

different internal - external port

Choose a reason for hiding this comment

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

That is usually done for reserved/assigned ports. In the case of port 8182 it is not assigned. If it ever conflicts with someone's local setup we can set this as an env var like

ports:
  - ${GREMLEX_SERVER_PORT}:8182

and use an .env file like

GREMLEX_SERVER_PORT=8182

Copy link

@yolo-vikram yolo-vikram left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@ignaciogoldchluk-yolo ignaciogoldchluk-yolo merged commit 122ea94 into master Feb 20, 2025
1 check passed
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.

3 participants