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.

Sign up for GitHub

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

Jump to bottom

Support for -with-password-authentication #43

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support for -with-password-authentication #43

wants to merge 3 commits into from

Conversation

macno
Copy link
Contributor

@macno macno commented Aug 2, 2020

Closes #37

@macno macno requested a review from gizero August 2, 2020 08:18
@gizero gizero changed the title Dupport for -with-password-authentication Support for -with-password-authentication Aug 3, 2020
@gizero
Copy link
Contributor

gizero commented Aug 3, 2020

A few comments (disclaimer: not a real review here):

  • commit 3353b85 looks unrelated. Would be nice to:
    1. have an issue tracking the issue behind this change and in a separate PR:
    2. have a (failing) test to show the (not obvious) effect when including the role multiple times
    3. a fix like the suggested one showing the test turning to green.
  • commit e05e5ed: why do we need a new scenario to test this? It's not obvious to me. Assuming a new scenario is what we really need, I'd suggest having a commit for molecule scaffolded content only followed by a commit implementing relevant changes for testing the new feature... I'm really struggling to figure out what is the "meat" here...

@macno macno force-pushed the macno/support-for-with-password-authentication branch from e05e5ed to 124b622 Compare August 4, 2020 05:58
@macno
Copy link
Contributor Author

macno commented Aug 4, 2020

A few comments (disclaimer: not a real review here):

  • commit 3353b85 looks unrelated. Would be nice to:

    1. have an issue tracking the issue behind this change and in a separate PR:
    2. have a (failing) test to show the (not obvious) effect when including the role multiple times
    3. a fix like the suggested one showing the test turning to green.

This commit is already present in PR #42 Removed from here

  • commit e05e5ed: why do we need a new scenario to test this? It's not obvious to me. Assuming a new scenario is what we really need, I'd suggest having a commit for molecule scaffolded content only followed by a commit implementing relevant changes for testing the new feature... I'm really struggling to figure out what is the "meat" here...

Not sure how to test in a single scenario the same role with different vars.
I'll try to follow your suggestion splitting the changes using the new scenario

@macno macno force-pushed the macno/support-for-with-password-authentication branch from 124b622 to fbcf17d Compare August 4, 2020 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gizero gizero Awaiting requested review from gizero

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for -with-password-authentication theo-agent install argument
2 participants
@macno @gizero