Skip to content
This repository has been archived by the owner on May 24, 2018. It is now read-only.

CDSK-904 - add codebases_arg to Trigger links #354

Merged
merged 4 commits into from
Apr 30, 2018

Conversation

warcholprzemo
Copy link

@warcholprzemo warcholprzemo commented Apr 16, 2018

Add missing codebase_arg
codebase_arg

@warcholprzemo warcholprzemo self-assigned this Apr 16, 2018
@coveralls
Copy link

coveralls commented Apr 16, 2018

Coverage Status

Coverage increased (+0.0008%) to 68.623% when pulling e0012c1 on CDSK-904-add-sourcestamps-to-Trigger-links into 0afd7cf on staging.

@warcholprzemo warcholprzemo force-pushed the CDSK-904-add-sourcestamps-to-Trigger-links branch from 882b17a to 6c0fc1f Compare April 16, 2018 09:25
@warcholprzemo warcholprzemo changed the title [WIP] CDSK-904 - add sourcestamps to Trigger links [WIP] CDSK-904 - add codebases_arg to Trigger links Apr 16, 2018
@warcholprzemo warcholprzemo changed the title [WIP] CDSK-904 - add codebases_arg to Trigger links CDSK-904 - add codebases_arg to Trigger links Apr 16, 2018

self.assertEqual(len(trigger_step_status.urls), 1)
self.assertEqual(len(trigger_links), 1)

Choose a reason for hiding this comment

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

Can we check this link? I mean checking the assertion value of the trigger_links element.

Copy link
Author

Choose a reason for hiding this comment

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

I can see 1 problem with checking values of trigger_links

  1. trigger_links[0] is not a string but is a 3-elements tuple. Data for this tuple comes from stubbed method getURLForBuild (+codebase_arg). And if I start checking values I won't do an unittest for real tuple but stub tuple

Maybe I should check if this tuple has exactly 3 elements instead of checking values?

brids = self.build.brids
db_results = yield self.build.builder.master.db.buildrequests\
.getBuildRequestForStartbrids(brids)
master = self.build.builder.master
for build in db_results:
friendly_name = master.status.getFriendlyName(build['buildername'])
url = master.status.getURLForBuild(build['buildername'], build['number'],
url = master.status.getURLForBuild(build['buildername'],

Choose a reason for hiding this comment

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

Is it possible to pass codebases_arg to the getURLForBuild method?
I think that since this method prepares URL already, we can add codebase_arg as nonrequired parameter and delegate to it the whole URL building process including GET parameters

Copy link
Author

Choose a reason for hiding this comment

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

I thought about it but getURLForBuild has already +getCodebasesArg(sourcestamps=sourcestamps)
If I add next parameter codebase_arg I need to do something with this computation of getCodebasesArg.
And then which parameter is more important? Can I use sourcestamps and codebase_arg together or not? Should I merge this 2 parameters before creating url params?

The simplest way was joining url with codebases_arg in prepare_trigger_links

@@ -380,27 +380,28 @@ def upgradeToVersion5(self):
self.wasUpgraded = True

@defer.inlineCallbacks
def prepare_trigger_links(self):
def prepare_trigger_links(self, codebases_arg):

Choose a reason for hiding this comment

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

minor: It would be nice to add a docstring

Copy link
Author

Choose a reason for hiding this comment

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

Done

@warcholprzemo warcholprzemo force-pushed the CDSK-904-add-sourcestamps-to-Trigger-links branch from d3da1b6 to 3894214 Compare April 30, 2018 09:19
@warcholprzemo warcholprzemo merged commit b3b82ce into staging Apr 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants