Skip to content

MQE-1600: MFTF Vault integration #382

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
Merged
merged 8 commits into from
Jul 24, 2019
Merged

MQE-1600: MFTF Vault integration #382

merged 8 commits into from
Jul 24, 2019

Conversation

jilu1
Copy link
Contributor

@jilu1 jilu1 commented Jul 16, 2019

Description

Fixed Issues (if relevant)

  1. magento/magento2-functional-testing-framework#<issue_number>: Issue title
  2. ...

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/verification tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)
  • Changes to Framework doesn't have backward incompatible changes for tests or have related Pull Request with fixes to tests

@coveralls
Copy link

coveralls commented Jul 17, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.5%) to 54.988% when pulling 80500e0 on MQE-1600 into fa56508 on develop.

KevinBKozan
KevinBKozan suggested changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@KevinBKozan KevinBKozan left a comment

Choose a reason for hiding this comment

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

Initial Review, overall implementation and design decisions are good, just needs clarification and some small fixes.

@KevinBKozan
Copy link
Contributor

Two things:

  • Need to prevent of printing out .env token via {{_ENV.<vault token}}
  • We need to prefer local .creds over remote in case of local development.

jilu1 added 3 commits July 19, 2019 13:46
@jilu1
MQE-1600: MQE-1600: MFTF Vault integration
c3f67ea 
- prefer credentials from local file over vault
- address review comments
@jilu1
MQE-1600: MFTF Vault integration
4c10d1c 
- prefer credentials from local file over vault
- address review comments
@jilu1
MQE-1600: MFTF Vault integration
36c5865 
- prefer credentials from local file over vault
- address review comments
@jilu1
MQE-1600: MFTF Vault integration
3f7fe82 
- Downgrade vault php version to be compatible to php 7.0
@jilu1
Copy link
Contributor Author

jilu1 commented Jul 22, 2019

Two things:

  • Need to prevent of printing out .env token via {{_ENV.<vault token}}
    I create a new story MQE-1647 to address this issue.
  • We need to prefer local .creds over remote in case of local development.
    Changed.

KevinBKozan
KevinBKozan suggested changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@KevinBKozan KevinBKozan left a comment

Choose a reason for hiding this comment

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

Changes look like they work, just need minor restructure for simplicity and code readability.

src/Magento/FunctionalTestingFramework/DataGenerator/Handlers/CredentialStore.php Outdated
} catch (TestFrameworkException $e) {
}
}

// Initialize file storage
try {
$this->credFile = new FileStorage();
$this->credStorage[self::ARRAY_KEY_FOR_FILE] = new FileStorage();
} catch (TestFrameworkException $e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Log the exception so that we can see that FileStorage failed to initialize: <message>. Do the same for Vault.

That way, if it fails in a build or something we can refer to the logs to see what did and didn't initialize properly.

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 think the exceptions are already logged in FileStorage and VaultStorage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but human readable statements will make the log more useful. Instead of just seeing a failure message, you see that the credential store is not using a specific method.

@jilu1
MQE-1600: MFTF Vault integration
6313da3 
- Simplify the logic to handle the storage array
KevinBKozan
KevinBKozan approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@KevinBKozan KevinBKozan left a comment

Choose a reason for hiding this comment

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

Initial integration looks good. Works with local, vault, and several mixtures as expected, fallback mechanisms work as required.

Are we including documentation on this change in this PR?

@jilu1 jilu1 merged commit 4212ad6 into develop Jul 24, 2019
KevinBKozan added a commit that referenced this pull request Jul 25, 2019
@KevinBKozan
Revert "Merge pull request #382 from magento/MQE-1600"
1359e26 
This reverts commit 4212ad6, reversing
changes made to fa56508.
KevinBKozan added a commit that referenced this pull request Jul 25, 2019
@KevinBKozan
Merge pull request #393 from magento/revert-MQE-1600
ba310f5 
Revert "Merge pull request #382 from magento/MQE-1600"
jilu1 added a commit that referenced this pull request Aug 5, 2019
@jilu1
Revert "Revert "Merge pull request #382 from magento/MQE-1600""
9a09a90 
This reverts commit 1359e26
soumyau pushed a commit that referenced this pull request Aug 9, 2019
@jilu1 @soumyau
Revert "Revert "Merge pull request #382 from magento/MQE-1600""
650e12f 
This reverts commit 1359e26
magento-devops-reposync-svc pushed a commit that referenced this pull request Mar 27, 2024
@KevinBKozan
Merge pull request #382 from magento-gl/ACQE-6085_ver1
77a4e0c 
ACQE-6085 : Static check for class and file name mismatch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KevinBKozan KevinBKozan KevinBKozan approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@jilu1 @coveralls @KevinBKozan