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

Make integration tests for credentials creation pass #10

Merged
merged 4 commits into from
Jul 19, 2017
Merged

Make integration tests for credentials creation pass #10

merged 4 commits into from
Jul 19, 2017

Conversation

olive42
Copy link

@olive42 olive42 commented Jul 11, 2017

  • Settle on the 'id' attribute as the attr_name for everything, since it is
    mandatory.
  • The xml method can now be shared (protected) with the parent class as it
    would be the same in every subclass.
  • Reduce the scope of the private key lookup. Due to the way Jenkins stores
    its credentials, it is quite hard to replicate this logic in integration
    tests without copying it entirely. As an exercise for later, stop reading
    credentials.xml and actually query Jenkins, but this is moving towards
    functional testing.

Description

Make us more confident that further changes in the credentials part of the cookbook do the right thing. This will be important for #9

Olivier Tharan added 2 commits July 11, 2017 15:16
- Settle on the 'id' attribute as the attr_name for everything, since it is
  mandatory.
- The xml method can now be shared (protected) with the parent class as it
  would be the same in every subclass.
- Reduce the scope of the private key lookup. Due to the way Jenkins stores
  its credentials, it is quite hard to replicate this logic in integration
  tests without copying it entirely. As an exercise for later, stop reading
  credentials.xml and actually query Jenkins, but this is moving towards
  functional testing.
@@ -6,30 +6,37 @@

# Test basic password credentials creation
jenkins_password_credentials 'schisamo' do
id 'schisamo'
username 'schisamo'

Choose a reason for hiding this comment

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

The username attribute is not required, since it is the name attribute. I would leave that out here as well and in the following resources.
Let's add it back on our own version of the cookbook.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@emmanuelguerin emmanuelguerin merged commit f6c7189 into criteo-forks:master Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants