meltano
meltano copied to clipboard
chore: Remove Makefile
Several of the commands in the Makefile do not work anymore, are not necessary, or should not be run locally. The Makefile has a considerable amount of dead code within it, and presents a small but significant burden to contributors. It all adds up: the Makefile does not prevent contributors from having to learn various poetry and yarn invocations to build/test/etc. our software - it just adds more on top of that.
From asking around, and searching through the repository, it seems like there are only 2 commonly used make
commands: ui
and bundle
. They have been replaced by scripts/bundle_ui.sh
, which can be run from anywhere to achieve the same effect.
Besides that, all of the local tasks that used to be carried out through the Makefile should now instead be done through poetry, yarn, and optionally pre-commit.
Much of what was in the Makefile was for CI, which is now all handled in the workflows and actions under .github/
. In the unlikely circumstance that we need to run those commands locally, due to the simplicity of how we have our CI set up, we can copy-paste the commands from the workflows/actions manually.
Relates to:
- #6262
- #6487
Deploy Preview for meltano ready!
Name | Link |
---|---|
Latest commit | bd9e858ca40025add33ac549eb49d711a64b790e |
Latest deploy log | https://app.netlify.com/sites/meltano/deploys/63b7b50550a5430009f1e115 |
Deploy Preview | https://deploy-preview-6488--meltano.netlify.app/getting-started/installation |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
I intend for the changes in this PR to function as a scream test. If none of us have a good reason to keep the Makefile around, then we should remove it on main
(by merging this PR) and see if that causes any problems. If not, then we continue on our merry way. Otherwise, we revert this change, and possibly revisit the topic with the knowledge we'll have gained from the problems that arose.
@WillDaSilva - Thanks for your work to proactively invest in cleaner and leaner build processes.
What the process doesn't really give us in this iteration is what actions we'd take if something went wrong or if a file gets committed to the repo which shouldn't have.
Also, for local UI dev and testing it seems likely that we'll accidentally commit files back to the repo which should not have been committed. (Or perhaps they are already git ignored?)
At least to keep functional parity with the make script and to address the above, can we add something like a clean.sh
and some readme or contributor doc with a few lines on what does what - especially which steps are needed to build a release and/or building for local tests? One approach is to just document the steps a maintainer would run if the process has to be executed outside of CI, including the cleanup afterwards.
cc @alexmarple and @rwfeather who've been doing the UI dev as of recently and who might have input into the build and cleanup steps.
Also - the makefile previously insulated us somewhat against having to invoke yarn directly and I'm not sure if that changes with this update and if we'd need to become more familiar with any specific commands in yarn to perform general dev/test/build/cleanup functions.
Codecov Report
Merging #6488 (c3d23d5) into main (0b09916) will decrease coverage by
0.03%
. The diff coverage isn/a
.
:exclamation: Current head c3d23d5 differs from pull request most recent head bd9e858. Consider uploading reports for the commit bd9e858 to get more accurate results
@@ Coverage Diff @@
## main #6488 +/- ##
==========================================
- Coverage 88.90% 88.86% -0.04%
==========================================
Files 294 293 -1
Lines 21537 21497 -40
Branches 1704 1702 -2
==========================================
- Hits 19147 19104 -43
- Misses 2024 2026 +2
- Partials 366 367 +1
Impacted Files | Coverage Δ | |
---|---|---|
src/meltano/core/db.py | 73.77% <0.00%> (-4.61%) |
:arrow_down: |
src/meltano/cli/elt.py | 93.85% <0.00%> (-1.12%) |
:arrow_down: |
src/meltano/core/error.py | 86.48% <0.00%> (-0.70%) |
:arrow_down: |
src/meltano/core/elt_context.py | 95.86% <0.00%> (-0.69%) |
:arrow_down: |
src/meltano/cli/config.py | 70.24% <0.00%> (-0.13%) |
:arrow_down: |
src/meltano/core/utils/__init__.py | 91.86% <0.00%> (-0.07%) |
:arrow_down: |
src/meltano/cli/__init__.py | 78.72% <0.00%> (ø) |
|
tests/meltano/core/test_utils.py | 100.00% <0.00%> (ø) |
|
tests/meltano/core/test_db_compat.py |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
@WillDaSilva Double checked the docs - the ui/api dev guides don't seem to mention the make files at all (🥳), I think the make lint
reference on https://docs.meltano.com/contribute/style might be the only spot.
the makefile previously insulated us somewhat against having to invoke yarn directly and I'm not sure if that changes with this update and if we'd need to become more familiar with any specific commands in yarn to perform general dev/test/build/cleanup functions.
@aaronsteers That's probably mostly covered by https://docs.meltano.com/contribute/ui or https://github.com/meltano/meltano/blob/main/src/webapp/README.md already, might just be worth having quick skim of those and see what's missing.
We can probably actually get rid of scripts/bundle_ui.py
, and just call build.py
instead, which does the same thing, but is automatically run by poetry when you build a wheel. Anyone who wishes to build & bundle the UI manually can simply manually call build.py
. Plus now we can have it as a Python file that just about everyone here will understand, rather than a bash script that might be intimidating to some.
I think the make lint reference on https://docs.meltano.com/contribute/style might be the only spot.
Thanks for catching that @pandemicsyn! I'll update it.
the makefile previously insulated us somewhat against having to invoke yarn directly and I'm not sure if that changes with this update and if we'd need to become more familiar with any specific commands in yarn to perform general dev/test/build/cleanup functions.
@aaronsteers In theory this was true, but in practice it seems like everyone was just calling poetry and yarn anyway.
What the process doesn't really give us in this iteration is what actions we'd take if something went wrong or if a file gets committed to the repo which shouldn't have.
I'm not sure what the concern is here.
Also, for local UI dev and testing it seems likely that we'll accidentally commit files back to the repo which should not have been committed. (Or perhaps they are already git ignored?)
The build artifacts are all already git ignored, so this shouldn't be an issue.
At least to keep functional parity with the make script and to address the above, can we add something like a
clean.sh
[...]
I'll add it as a Python script to provide Windows compatibility, and to have it be in a familiar language for our contributors. Overall I want to minimize the number of scripts we have, but this one seems self-explanatory and useful.
[can we add] some readme or contributor doc with a few lines on what does what [...]
The docs already generally say what to do using poetry and yarn, rather than the Makefile, but I can do another pass through and see what can be improved on that front.
[...] especially which steps are needed to build a release and/or building for local tests? One approach is to just document the steps a maintainer would run if the process has to be executed outside of CI, including the cleanup afterwards.
I believe having such a document is dangerous. When it comes to something as important and precise as releasing to PyPI, it should only be done in CI, and there should be a single source of truth about what to do, which is the code that runs in CI. If something goes horribly wrong and we need to do a release outside of CI, the steps to take can be summarized as "do exactly what CI does with as few deviations as possible".
Blocked by https://github.com/meltano/meltano/pull/6484, because we now run yarn && yarn build
when we build the wheel.
@edgarrmondragon @aaronsteers When Poetry uses build.py
, it builds a platform-specific wheel, rather than a pure wheel.
The offending code: https://github.com/python-poetry/poetry-core/blob/d5db91af41e0befd413ce54190bfb7fb7fed10f1/src/poetry/core/masonry/builders/wheel.py#L390
Related issues:
- https://github.com/python-poetry/poetry/issues/3594
- https://github.com/python-poetry/poetry/issues/2051
We can't use build.py
until this has been resolved. I'm going to ask the Poetry core maintainers on Discord about these issues, and see if I can get things moving along. Until we can use build.py
with Poetry to build a pure wheel, this PR will be a draft.