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

Small improvements to make setup process easier for OS contributors #1171

Merged

Conversation

AndrewDryga
Copy link
Collaborator

For reasoning on each change please see the commit messages.

For umbrella apps everything goes into /_build directory so there no need to ignore directories that should never be created
…pect

1. We want ecto.create and ecto.migrate to be run on each tests, this will simplify setup steps (no need to run migrations manually)

2. ecto.remigrate is not needed because now you can just run ecto.drop and on tests migrations would be executed anyways.
@AndrewDryga
Copy link
Collaborator Author

Should I remove or update scripts/uninstall.sh? Right now it only works for installation from package managers and the most common way to install is Docker, so maybe we don't want to have scripts nobody is using to not make people confused (like I was).

@AndrewDryga
Copy link
Collaborator Author

AndrewDryga commented Dec 3, 2022

Does anyone have a hint where this is coming from on each mix test and mix.phx server run?

$ mix phx.server
usage: route [-dnqtv] command [[modifiers] args] # <-------
[debug] .....

UPD: Found it: https://github.com/firezone/firezone/blob/master/config/dev.exs#L34.

@jamilbk
Copy link
Member

jamilbk commented Dec 5, 2022

Wow — thanks for the contribution! Traveling today but should have some time to review later.

Should I remove or update scripts/uninstall.sh? Right now it only works for installation from package managers and the most common way to install is Docker, so maybe we don't want to have scripts nobody is using to not make people confused (like I was).

It may be better to rename the script scripts/omnibus_uninstall.sh (and update docs at https://docs.firezone.dev/administer/uninstall/ ) as we still have a large number of Omnibus instances in the wild.

@coveralls
Copy link

coveralls commented Dec 5, 2022

Pull Request Test Coverage Report for Build 82465e91e97b323bb93611e7866abb85a93e7b7a-PR-1171

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 66.335%

Files with Coverage Reduction New Missed Lines %
apps/fz_common/lib/fz_kernel_version.ex 2 0%
Totals Coverage Status
Change from base Build f4cc03454eff68aa000f44b2101d779ed06c37dd: -0.2%
Covered Lines: 1200
Relevant Lines: 1809

💛 - Coveralls

@AndrewDryga AndrewDryga changed the title Small improvements to make setup process easier for OS contributors [WIP] Small improvements to make setup process easier for OS contributors Dec 5, 2022
This an is opinionated change. Right now devcontainer doesn't work but should be easy to fix (with renaming step name), but at the same time it forces developers that use VS code to have unified development environment (including plugins for the editor itself).

I feel like it's not a good path to go for OS and for small team - everyone should be allowed to use setup they like. Especially for people like me that tend to recompile ls-elixir for Elixir plugin from master branch.

Plus it's yet another thing to maintain while nobody on the team is using it, which means it will be always causing issues.
Otherwise I'm getting this on my MacOS (that has a `route` implementation that doesn't show interfaces) when `mix phx.server` is executed:
```
usage: route [-dnqtv] command [[modifiers] args]
```
Both public_key and name are unique and we should not use static values for field covered by unique index, otherwise deadlocks and slow tests are expected.
The changeset code doesn't have any code that accesses the database and individual Ecto.SQL commands are already wrapped in transactions by default, so there is no need to start it manually and hold for longer than expected (while irrelevant Elixir code is running).
@AndrewDryga AndrewDryga force-pushed the andrew/os-quality-of-life-improvements branch from 7d18f7f to bc354ca Compare December 5, 2022 22:59
Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple notes

CONTRIBUTING.md Show resolved Hide resolved
config/dev.exs Show resolved Hide resolved
.github/workflows/omnibus_build.yml Outdated Show resolved Hide resolved
apps/fz_http/lib/fz_http/devices.ex Show resolved Hide resolved
@AndrewDryga AndrewDryga changed the title [WIP] Small improvements to make setup process easier for OS contributors Small improvements to make setup process easier for OS contributors Dec 6, 2022
Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

LGTM

@jamilbk jamilbk merged commit 28fe571 into firezone:master Dec 6, 2022
@AndrewDryga AndrewDryga deleted the andrew/os-quality-of-life-improvements branch December 7, 2022 20:04
@AndrewDryga
Copy link
Collaborator Author

❤️

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