Add Invoke tasks for all Digital Marketplace repos #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lfdebrux
added a commit
to Crown-Commercial-Service/digitalmarketplace-utils
that referenced
this pull request
Dec 30, 2020
Why --- We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences. This commit replaces Make with invoke, using the tasks from alphagov/digitalmarketplace-dev-tools. See Crown-Commercial-Service/digitalmarketplace-developer-tools#1 for more details.
lfdebrux
force-pushed
the
ldeb-add-invoke
branch
from
9156c1d to
730f22b
Compare
lfdebrux
added a commit
to Crown-Commercial-Service/digitalmarketplace-api
that referenced
this pull request
Dec 30, 2020
We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences. This commit replaces Make with invoke, using the tasks from alphagov/digitalmarketplace-dev-tools. See Crown-Commercial-Service/digitalmarketplace-developer-tools#1 for more details.
lfdebrux
added a commit
to Crown-Commercial-Service/digitalmarketplace-api
that referenced
this pull request
Dec 30, 2020
We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences. This commit replaces Make with invoke, using the tasks from alphagov/digitalmarketplace-developer-tools. See Crown-Commercial-Service/digitalmarketplace-developer-tools#1 for more details.
lfdebrux
added a commit
to Crown-Commercial-Service/digitalmarketplace-utils
that referenced
this pull request
Dec 30, 2020
We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences. This commit replaces Make with invoke, using the tasks from alphagov/digitalmarketplace-developer-tools. See Crown-Commercial-Service/digitalmarketplace-developer-tools#1 for more details.
bjgill
reviewed
Jan 6, 2021
There was a problem hiding this comment.
This looks like a good plan to me. We're primarily Python developers, not C developers. So despite invoke being new to us, I think it should be easier for us to maintain than our Makefiles.
My vote is to trial this for some repos and then roll it out to all if successful.
(had a quick skim through the code and had a couple of questions - doesn't affect my overall conclusion)
|
|
||
| @task(show_environment, virtualenv) | ||
| def run_app(c): | ||
| """Run app""" |
There was a problem hiding this comment.
Style nit - do all the doc comments add value? I think it's fine to not have a doc comment if the task name is self-documenting.
There was a problem hiding this comment.
I added them mainly because they show up as help text when you run invoke --list. I agree that currently they don't add much though :)
| @@ -0,0 +1,5 @@ | |||
| from dmdevtools.invoke_tasks import library_tasks as ns | |||
There was a problem hiding this comment.
Ah, I should document this somewhere. ns is a magic name for invoke, if it finds a Collection called ns or namespace in tasks.py it will use that for the list of tasks for that directory.
There was a problem hiding this comment.
Link to invoke documentation elucidating further http://docs.pyinvoke.org/en/stable/concepts/namespaces.html#starting-out
Why --- We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences. What this commit does --------------------- This commit creates the infrastructure to replace make with [invoke] in all our repos. Because invoke is Python-based we can share tasks between repos using pip, reducing code duplication and reducing the effort needed to make a change to developer tooling across all repos at once. This commit adds tasks from digitalmarketplace-buyer-frontend, digitalmarketplace-api, and digitalmarketplace-utils, and groups the tasks together into `invoke.Collection`s that can be used in the `tasks.py` files for other repos. The tasks should perform exactly as they did with Make previously; however the hope is that once all repos are using this method improvements to the tooling can be made quickly and easily. For instance, I would like to reduce the number of tasks and make them quicker to run by caching more aggressively :D What happens next ----------------- In this paradigm `Makefile` and `tasks.py` are simply stubs that respectively install and import the tasks from digitalmarketplace-utils. All repos will have exactly the same `Makefile`, and `tasks.py` will select the appropriate collection for the repo (and make any further customisations if needed). Once all repos are using this method `make bootstrap` in any repo will install dmutils and invoke globally, so that changes to tasks are shared across all repos on a developers laptop at once. [invoke]: https://pyinvoke.org
This change makes it simpler to call commands that are in the the venv; they will now take precedence (as if `venv/bin/activate` had been called).
lfdebrux
force-pushed
the
ldeb-add-invoke
branch
from
4b9e1b0 to
ce2f087
Compare
lfdebrux
changed the title
bjgill
approved these changes
Feb 2, 2021
|
@lfdebrux - who has write/admin access to this repo? |
Oh woops, forgot to set that. Digital Marketplace team now has admin access. |
lfdebrux
added a commit
to Crown-Commercial-Service/digitalmarketplace-utils
that referenced
this pull request
Feb 22, 2021
We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences. This commit replaces Make with invoke, using the tasks from alphagov/digitalmarketplace-developer-tools. See Crown-Commercial-Service/digitalmarketplace-developer-tools#1 for more details.
lfdebrux
added a commit
to Crown-Commercial-Service/digitalmarketplace-utils
that referenced
this pull request
Feb 23, 2021
We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences. This commit replaces Make with invoke, using the tasks from alphagov/digitalmarketplace-developer-tools. See Crown-Commercial-Service/digitalmarketplace-developer-tools#1 for more details.
lfdebrux
added a commit
to Crown-Commercial-Service/digitalmarketplace-utils
that referenced
this pull request
Feb 26, 2021
We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences. This commit replaces Make with invoke, using the tasks from alphagov/digitalmarketplace-developer-tools. See Crown-Commercial-Service/digitalmarketplace-developer-tools#1 for more details.
lfdebrux
added a commit
to Crown-Commercial-Service/digitalmarketplace-api
that referenced
this pull request
Feb 26, 2021
We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences. This commit replaces Make with invoke, using the tasks from alphagov/digitalmarketplace-developer-tools. See Crown-Commercial-Service/digitalmarketplace-developer-tools#1 for more details.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Ticket: https://trello.com/c/hCCo28Bc/1329-add-makefiles-to-digitalmarketplace-utils-and-reduce-duplication-across-apps
We want to be able to reduce duplication of code across our repos; one of the places we have a lot of duplicated code is in our Makefiles; all of our repos have very similar Makefiles with small historical differences.
What this pull request does
This PR creates the infrastructure to replace make with invoke in all our repos. Because invoke is Python-based we can share tasks between repos using pip, reducing code duplication and reducing the effort needed to make a change to developer tooling across all repos at once.
This PR adds tasks from digitalmarketplace-buyer-frontend, digitalmarketplace-api, and digitalmarketplace-utils, and groups the tasks together into
invoke.Collections that can be used in thetasks.pyfiles for other repos. The tasks should perform exactly as they did with Make previously; however the hope is that once all repos are using this method improvements to the tooling can be made quickly and easily. For instance, I would like to reduce the number of tasks and make them quicker to run by caching more aggressively :DWhat happens next
In this paradigm
Makefileandtasks.pyare simply stubs that respectively install and import the tasks from digitalmarketplace-utils. All repos will have exactly the sameMakefile, andtasks.pywill select the appropriate collection for the repo (and make any further customisations if needed).See the following draft PRs for examples of how this will be done:
Once all repos are using this method
make bootstrapin any repo will install dmutils and invoke globally, so that changes to tasks are shared across all repos on a developers laptop at once.Further questions
Why install dmdevtools globally?
I figure that having developer tooling installed globally (outside of a venv) means we can quickly get the latest features across all repos instantly, without having to make a whole bunch of PRs and releases. Plus it makes the bootstrap process a little simpler.
Why invoke instead of Make?
For me the main advantage of invoke is that we can use Python's package management to share code; sharing makefiles between repos is not an easy thing to do, and usually ends up going down the git submodules route, which I wanted to avoid. Also, while Make does have methods for including other makefiles, you lose tab-completion and other help features when all the targets are defined outside of the main Makefile.
Why another repo?
I looked at adding the code in this PR to alphagov/digitalmarketplace-utils, however I found that this lead to a large number of app dependencies including Flask being installed alongside the developer tooling. While this might be okay in an app venv or even on a developer laptop (although a little annoying), this got especially annoying with CI pipelines.