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

refactoring add/set methods #173

Merged
merged 1 commit into from Jul 12, 2017

Conversation

Projects
None yet
2 participants
@laurafitzgerald

laurafitzgerald commented Jul 11, 2017

Changes
Rename the methods

  • setEnvVars

  • setAnnotations

  • setImagePullSecrets
    to use add syntax

  • added methods for setting. i.e. overwrite the existing array.

Motivation
The names of the methods currently set<Object> indicates that you are setting a new value rather than adding to an existing array.

@laurafitzgerald

This comment has been minimized.

Show comment
Hide comment
@laurafitzgerald

laurafitzgerald Jul 11, 2017

ping @carlossg.
If you could take a look at this when you get some time. You seem active on this repo so pinging you but feel free to ping a more relevant person if required. ;)

laurafitzgerald commented Jul 11, 2017

ping @carlossg.
If you could take a look at this when you get some time. You seem active on this repo so pinging you but feel free to ping a more relevant person if required. ;)

@carlossg carlossg merged commit 5043bcf into jenkinsci:master Jul 12, 2017

@carlossg

This comment has been minimized.

Show comment
Hide comment
@carlossg

carlossg commented Jul 12, 2017

thanks!

carlossg added a commit that referenced this pull request Jul 17, 2017

@carlossg

This comment has been minimized.

Show comment
Hide comment
@carlossg

carlossg Jul 17, 2017

The PR didn't even compile 😢

carlossg commented Jul 17, 2017

The PR didn't even compile 😢

@laurafitzgerald

This comment has been minimized.

Show comment
Hide comment
@laurafitzgerald

laurafitzgerald Jul 17, 2017

@carlossg this is my first pr to this repo. Is there some step that I missed for verification?

laurafitzgerald commented Jul 17, 2017

@carlossg this is my first pr to this repo. Is there some step that I missed for verification?

@carlossg

This comment has been minimized.

Show comment
Hide comment
@carlossg

carlossg Jul 18, 2017

yeah sorry I merged too quickly, and there should be a CI test run, but you can run mvn clean install to check that the build and test passes.

carlossg commented Jul 18, 2017

yeah sorry I merged too quickly, and there should be a CI test run, but you can run mvn clean install to check that the build and test passes.

@laurafitzgerald

This comment has been minimized.

Show comment
Hide comment
@laurafitzgerald

laurafitzgerald Jul 19, 2017

@carlossg cool. thanks for the info. will do that in future

laurafitzgerald commented Jul 19, 2017

@carlossg cool. thanks for the info. will do that in future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment