Juju Lint

Merge ~mertkirpici/juju-lint:lp/1989575 into juju-lint:master

  1. Git
  2. lp:~mertkirpici/juju-lint
  3. lp/1989575
  4. Merge into master
Proposed by Mert Kirpici
Status: Merged
Approved by: Eric Chen
Approved revision: 1e67b7a4a726e02e3ac82699562e1db75f8ff6ac
Merged at revision: c05a3913ecaabbc5ba41cf14895ffb7ced946567
Proposed branch: ~mertkirpici/juju-lint:lp/1989575
Merge into: juju-lint:master
Diff against target: 159 lines (+37/-29)
5 files modified
.gitignore (+1/-0)
Makefile (+1/-3)
tests/functional/requirements.txt (+2/-1)
tests/unit/requirements.txt (+1/-12)
tox.ini (+32/-13)
Reviewer Review Type Date Requested Status
Eric Chen Approve
Gabriel Cocenza Approve
Review via email: mp+429879@code.staging.launchpad.net

Commit message

Close LP #1989575

Description of the change

After the renaming process in 30066d3b[1], makefile dev-environment target was overlooked, ending up with the broken target.

To fix this properly, rather than hardcoding tests/unit/requirements.txt into the makefile, we introduce requirements-dev.txt file and point the unittest requirements file to that.

Since this issue[0] is resolved, unpinned the flake8 version.

Also the .venv/ created by dev-environment was not ignored by git, fixed that.

[0] https://github.com/csachs/pyproject-flake8/issues/13
[1] https://git.launchpad.net/juju-lint/commit/?id=30066d3b9d49f4c673cf9766d13054b735c26a30

To post a comment you must log in.
Revision history for this message
Mert Kirpici (mertkirpici) wrote :

make lint and unittests:
https://pastebin.ubuntu.com/p/gtcHgKD8Fx/

Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
Eric Chen (eric-chen) wrote :

Please provide functional test log too.

review: Needs Information
Revision history for this message
Mert Kirpici (mertkirpici) wrote :

lint + unittests + functional:
https://pastebin.ubuntu.com/p/tF7CKBwzQz/

Revision history for this message
Gabriel Cocenza (gabrielcocenza) wrote :

I liked the idea of having a single requirements-dev.txt at the "principal" folder of the project.

With that said, is there a specific reason why the functional-tests requirements it's not in the same file?

If we have a single requirements-dev.txt it won't be necessary to have the unit and functional requirements.txt and the [testenv] on the .tox file could be responsible to install all dependencies of the project. With that, I guess we can remove deps from [testenv:func], [testenv:func-smoke], [testenv:func-dev] and [testenv:pre-commit].

review: Needs Information
Revision history for this message
Mert Kirpici (mertkirpici) wrote :

So after giving this some thought and experimenting, I came to a conclusion that having a central dev requirements ends up installing ALL of the dependencies for every tox environment, which is very redundant and takes a lot of time and space.

Therefore I moved away from that idea towards the opposite.
With the second change I pushed, every tox env installs only their own dependencies they need. (Most charms also follow this)

Also another change is that, the virtual env creation in makefile's `dev-environment` target was not being created by tox like all the rest but it was using the package `virtualenv`. I moved away from that also, in favor of using tox for the `dev-environment` creation as well.

Here is a showcase of all current make+tox targets :)
https://pastebin.ubuntu.com/p/ZkxG8mFsWV/

Revision history for this message
Martin Kalcok (martin-kalcok) wrote :

overall LGTM. just a small note. You can make multiple tox environments share the same "virtual environment folder" to cut down on venv duplicity. For example how "format-code" shares "lint" venv in juju-verify [0]. Same could be applied here.

[0] https://github.com/canonical/juju-verify/blob/237fea58fd579279121f476d244bacbd1992355c/tox.ini#L43

Revision history for this message
Gabriel Cocenza (gabrielcocenza) wrote :

Generally this patch LGTM, but the command `make pre-commit` it's not working.

review: Needs Information
Revision history for this message
Mert Kirpici (mertkirpici) wrote :

pushed update to recover the pre-commit target

Revision history for this message
Gabriel Cocenza (gabrielcocenza) wrote :

LGTM. Please provide the logs of tests before merging.

review: Approve
Revision history for this message
Mert Kirpici (mertkirpici) wrote :

updated one last time. changes:
- rebased onto current master.
- squashed commits.
- rewritten commit message

`make test`:
https://pastebin.ubuntu.com/p/WdZ8q9j5nC/

all green. ready to land.

Revision history for this message
Eric Chen (eric-chen) :
review: Approve
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

Change successfully merged at revision c05a3913ecaabbc5ba41cf14895ffb7ced946567

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches