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

Allow molecule to list scenarios present under molecule directory within a collection #3989

Merged
merged 3 commits into from
Aug 8, 2023

Conversation

audgirka
Copy link
Contributor

@audgirka audgirka commented Aug 7, 2023

Related: #3987

Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

The fact this this passed packaging worries me because we added a new dependency which we failed to declare. That job should have failed. We better find a way to make it fail if we do this to avoid adding hidden deps. I am trying to figure-out how to do this properly...

@cidrblock
Copy link
Contributor

cidrblock commented Aug 7, 2023

yeah, looks like it got pulled in from ansible-lint (for the tests)
https://github.com/ansible/molecule/actions/runs/5785724537/job/15678953688#step:9:369

I think we should be able to do this without wcmatch though......or at least wait until we get a issue that can';t be resolved without it.

@ssbarnea
Copy link
Member

ssbarnea commented Aug 7, 2023

I told @ajinkyau to use wcmatch, that is not an issue. The issue is that we did not declare it as a direct dependency.

Second issue is why it got installed as I doubt we want to install ansible-lint in normal testing environments. I am still looking for a tool that can help us detect missing direct dependencies but i did not find one that works with modern packaging.

@cidrblock
Copy link
Contributor

ansible-lint is a dep for test........could we remove that?

I'll look into wcamtch, I'm not familiar enough with it to grok what it does above the std lib

@cidrblock
Copy link
Contributor

cidrblock commented Aug 7, 2023

related, navigator had a similar issue a while back, where a dep was satisfied only in the tests, this was added as a check

https://github.com/ansible/ansible-navigator/blob/c5a8056c22b73e03686ae8d0655053d42a64f1a0/tox.ini#L115

The goal being to run through some basic functionality, enough to hit the import statements across the codebase to flesh out anything missing in the base reqs

@audgirka
Copy link
Contributor Author

audgirka commented Aug 7, 2023

@ssbarnea @cidrblock Since we've removed support for the lint subcommand, I think we can remove ansible-lint as test dependency.
Tested locally and the tests are passing.

@audgirka audgirka requested a review from ssbarnea August 8, 2023 12:51
@audgirka audgirka merged commit 6daea25 into main Aug 8, 2023
@audgirka audgirka deleted the enhancement/scenario branch August 8, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants