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

molecule requirementes should not pinned #1831

Closed
ssbarnea opened this issue Mar 13, 2019 · 5 comments
Closed

molecule requirementes should not pinned #1831

ssbarnea opened this issue Mar 13, 2019 · 5 comments
Assignees
Labels
bug test Improvement to quality assurance: CI/CD, testing, building

Comments

@ssbarnea
Copy link
Member

Desired Behavior

Molecule is a development/testing tool and it should not have pinned runtime or testing requirements.

Instead we need to use ranges that prevent usage of dependencies what we know are bugged or broken.

This also should avoid possible conflicts between a molecule dependency would come into conflict with the version mentioned by ansible or another dependency.

Sooner or later we will clearly have a different version of foo package when using one version of anasible and another one when using a newer version.

When implementing this we should also use pip check to validate that after installation there are no reported conflicts reported by pip.

@ssbarnea ssbarnea added the bug label Mar 13, 2019
@ssbarnea ssbarnea self-assigned this Mar 13, 2019
@ssbarnea ssbarnea added the test Improvement to quality assurance: CI/CD, testing, building label Mar 13, 2019
@ssbarnea
Copy link
Member Author

@webknjaz @decentral1se @gundalow Please use up/down to confirm the desrcribed behavior.

@webknjaz
Copy link
Member

@ssbarnea in general, you're right. I'd even suggest using pyup bot or dependabot to automate keeping things up-to-date.

I'm not sure whether @gundalow or someone recorded this somewhere, but current dependency spec is messed up: it has both runtime and dev deps pinned and mixed in the single unmanageable file. It's one of the caveats of pbr which I hope to solve by removing it.

Projects usually have different types of dependencies:

  1. runtime deps, they should be open and have a lower limit + maybe a higher limit to next major if a certain dependency follows semver.
  2. env deps, they describe and pin all packages including direct dev/test deps and their transitive dependencies. They can also specify hashes for deps, which makes the thing even more resilient.

These two categories should be dealt with separately. It's fine to pin dev deps versions as long as we have an automated process of bumping them. It's better than keeping them without any limits since automatic bumping triggers PR+CI meaning that we'd have recorded logs of what happens with the project with new deps instead of cryptic bug reports post-factum.
We just need to be more careful with runtime deps and try to not shoot in own legs...

@webknjaz
Copy link
Member

webknjaz commented Mar 13, 2019

Ref: #1710 (comment)

ssbarnea added a commit that referenced this issue Mar 13, 2019
Also adds a `pip check` execution for testing that there are no
conflicts after instalation of all requirements.

Temporary the exit code returned by `pip check` is ignored as we have
a known conflict caused by `ansible[azure]` dependency.

Avoids installing lxc on MacOS(Darwin) to avoid test failures as lxc
does not even compile on that platform.

Forces pytest<4.0.0 because one plugin does not support it yet but
includes comment so we can fix that as soon the plugin is updated.

Fixes: #1831
Signed-off-by: Sorin Sbarnea <[email protected]>
ssbarnea added a commit that referenced this issue Mar 13, 2019
Also adds a `pip check` execution for testing that there are no
conflicts after instalation of all requirements.

Temporary the exit code returned by `pip check` is ignored as we have
a known conflict caused by `ansible[azure]` dependency.

Avoids installing lxc on MacOS(Darwin) to avoid test failures as lxc
does not even compile on that platform.

Forces pytest<4.0.0 because one plugin does not support it yet but
includes comment so we can fix that as soon the plugin is updated.

Fixes: #1831
Signed-off-by: Sorin Sbarnea <[email protected]>
@decentral1se decentral1se changed the title molecule requirmentes should not pinned molecule requirementes should not pinned Mar 14, 2019
@micheelengronne
Copy link
Contributor

Merge request: #2024

@decentral1se
Copy link
Contributor

I think we largely dealt with the worst of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug test Improvement to quality assurance: CI/CD, testing, building
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants