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

Issue/3110 modules v2 moduletool commands #3473

Closed
wants to merge 21 commits into from

Conversation

sanderr
Copy link
Contributor

@sanderr sanderr commented Nov 18, 2021

Description

Reviewed all moduletool commands for v2 compatibility.

closes #3110

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

  • Attached issue to pull request
  • Changelog entry
  • Type annotations are present
  • Code is clear and sufficiently documented
  • No (preventable) type errors (check using make mypy or make mypy-diff)
  • Sufficient test cases (reproduces the bug/tests the requested feature)
  • Correct, in line with design
  • End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )

Reviewer Checklist:

  • Sufficient test cases (reproduces the bug/tests the requested feature)
  • Code is clear and sufficiently documented
  • Correct, in line with design

src/inmanta/module.py Outdated Show resolved Hide resolved
raise Exception(f"Directory {module_dir} already exists")
cookiecutter(
"https://github.com/inmanta/inmanta-module-template.git",
checkout="v2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we perform a checkout from master? The intention is to have the V2 template on the master branch after the release, no? It looks like no v2 branch exist at the moment. Was this functionality tested?

Copy link
Contributor

Choose a reason for hiding this comment

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

I expect we will need the V1 a little longer until we have set up the proper distribution channels for v2 modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd discussed both this and #3473 (comment) with Wouter yesterday but I'd forgotten to share the conclusions. The main reason we need to keep v1 on master for now is that new v2 modules require a v2 std, which we haven't decided how to distribute yet. The v2 branch does not exist yet but this is part of inmanta/inmanta-module-template#13.

matches = version == reqv
editable = True
else:
reqv = ",".join(req.version_spec_str() for req in specs[name] if req.specs) or "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

The usage of reqv is inconsistent between V1 and V2 modules.

  • For V1 modules, reqv contains a list of version that match the version spec
  • For V2 modules, reqv contains the version specs.

Isn't this strange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree somewhat strange, though both Wouter and I are of the opinion that the v2 format is an improvement. I opted to stick with the stricter approach for v1 because it needs to take the compiler version into account, making it more complex than just the version specs. We could clean that up as well but I'm not sure it's worth the effort for v1.
A small remark though, for v1 modules, if I understand correctly, it never contains a list, always a single version: the latest version with a compatible compiler version constraint.


version_length = max(len(version), len(reqv), version_length)

table.append((name, version, reqv, version == reqv))
table.append((name, generation, editable, version, reqv, matches))
Copy link
Contributor

Choose a reason for hiding this comment

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

The version_length variable is not used anywhere. We can clean that up in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@arnaudsjs arnaudsjs left a comment

Choose a reason for hiding this comment

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

See review comments.

Copy link
Contributor

@wouterdb wouterdb left a comment

Choose a reason for hiding this comment

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

Why no testcases?

description: Reviewed all moduletool commands for v2 compatibility
change-type: major
sections:
deprecation-note: "The `inmanta module list -r` command has been deprecated"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps also mention the alternative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

raise Exception(f"Directory {module_dir} already exists")
cookiecutter(
"https://github.com/inmanta/inmanta-module-template.git",
checkout="v2",
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect we will need the V1 a little longer until we have set up the proper distribution channels for v2 modules.

sanderr and others added 4 commits November 19, 2021 09:59
Co-authored-by: arnaudsjs <2684622+arnaudsjs@users.noreply.github.com>
Copy link
Contributor

@wouterdb wouterdb left a comment

Choose a reason for hiding this comment

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

testcases! (I comment so you can re-request when you are done)

@sanderr
Copy link
Contributor Author

sanderr commented Nov 19, 2021

Also from the demo meeting:

  • check matches behavior for release mode master has not been changed by this PR
  • mention deprecation in inmanta module list --help output
  • Generation -> module type
  • 1/0 -> yes/no

@sanderr
Copy link
Contributor Author

sanderr commented Nov 22, 2021

Re-requesting review because of fundamental changes:

  • Dropped --force-reinstall from module install command as discussed on slack. Modified test cases to be compatible.
  • Extended Project.requires to include v2 requirements from requirements.txt

Non-fundamental changes:

)

cwd = os.getcwd()
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was meant to wrap the whole block below, but apparently it's not required because snippetcompiler does Project.set. I'll remove it.

@sanderr sanderr added the merge-tool-ready This ticket is ready to be merged in label Nov 23, 2021
@inmantaci
Copy link
Contributor

Processing this pull request

@inmantaci
Copy link
Contributor

Merged into branches master in b27d170

inmantaci pushed a commit that referenced this pull request Nov 23, 2021
# Description

Reviewed all moduletool commands for v2 compatibility.

closes #3110

# Self Check:

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [x] Attached issue to pull request
- [x] Changelog entry
- [x] Type annotations are present
- [x] Code is clear and sufficiently documented
- [x] No (preventable) type errors (check using make mypy or make mypy-diff)
- [x] ~~Sufficient test cases (reproduces the bug/tests the requested feature)~~
- [x] Correct, in line with design
- [x] ~~End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )~~

# Reviewer Checklist:

- [ ] Sufficient test cases (reproduces the bug/tests the requested feature)
- [ ] Code is clear and sufficiently documented
- [ ] Correct, in line with design
@inmantaci inmantaci closed this Nov 23, 2021
@inmantaci inmantaci deleted the issue/3110-modules-v2-moduletool-commands branch November 23, 2021 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-tool-ready This ticket is ready to be merged in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make sure all moduletool commands work for both V1 and V2 or report that they don't support one or the other
4 participants