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

Payload params #31

Merged
merged 6 commits into from Jun 17, 2014

Conversation

Projects
None yet
2 participants
@xaniasd
Copy link
Contributor

xaniasd commented Jun 17, 2014

This is a basic way to provide payload data as parameters to the build. In this case the repository_id is added to the build. Comes together with a couple of tests.

expect(subject.commits[1].message).to eq("fixed readme")
end

it "memoizes the result" do

This comment has been minimized.

@elvanja

elvanja Jun 17, 2014

Owner

I'd still like to keep memoize, see http://en.wikipedia.org/wiki/Memoization.

@@ -18,7 +18,7 @@ def with(project, details)
# @see hudson.model.AbstractProject#getDefaultParametersValues
parameters_values = project.get_default_parameters.reject { |parameter| parameter.name == branch_parameter.name }.collect { |parameter| parameter.getDefaultParameterValue() }.reject { |value| value.nil? }
parameters_values << StringParameterValue.new(branch_parameter.name, details.branch)

parameters_values << StringParameterValue.new('repository_id', details.repository_id)
ParametersAction.new(parameters_values)

This comment has been minimized.

@elvanja

elvanja Jun 17, 2014

Owner

Just please put an empty line between parameters_values and construction of ParametersAction.

raise NotFoundException.new("no project references the given repo url and commit branch") if projects.empty?

projects
end

def logger

This comment has been minimized.

@elvanja

elvanja Jun 17, 2014

Owner

Can you please explain where is this used? Does it show up in Jenkins logs as this class?

This comment has been minimized.

@xaniasd

xaniasd Jun 17, 2014

Author Contributor

not used, will remove

@elvanja

This comment has been minimized.

Copy link
Owner

elvanja commented Jun 17, 2014

Nice work, thanks! There are a few minor things to resolve, see the comments. Let's fix that and I'll merge.

@xaniasd

This comment has been minimized.

Copy link
Contributor Author

xaniasd commented Jun 17, 2014

Done!

@elvanja

This comment has been minimized.

Copy link
Owner

elvanja commented Jun 17, 2014

Cool, just noticed one more thing.
parse_request_spec.rb references default_payload.json while you added the push_payload.json. It would be better to just update the default one with push data (and remove push payload) and make sure all specs are passing. That way we'd have default and merge payload, which seems OK.

@xaniasd

This comment has been minimized.

Copy link
Contributor Author

xaniasd commented Jun 17, 2014

..aaand done, All specs are passing.

elvanja added a commit that referenced this pull request Jun 17, 2014

Merge pull request #31 from xaniasd/payload_params
Payload params enriched with project_id.

@elvanja elvanja merged commit e311899 into elvanja:master Jun 17, 2014

@xaniasd xaniasd deleted the xaniasd:payload_params branch Jun 17, 2014

@elvanja elvanja added the enhancement label Jul 25, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.