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

[Feat] apply/sequenced group charts dep #74

Merged

Conversation

gardlt
Copy link
Contributor

@gardlt gardlt commented Jun 19, 2017

What is the purpose of this pull request?:

We want to bring in sequenced deployments to the armada files

What issue does this pull request address?: Fixes #61

Notes for reviewers to consider:

Specific reviewers for pull request:
@theyer @drewwalters96

@gardlt gardlt force-pushed the feat/apply/sequenced-group-charts-dep branch 4 times, most recently from 515977b to 4131a41 Compare June 23, 2017 21:47
@gardlt gardlt changed the title [WIP]Feat/apply/sequenced group charts dep [Feat] apply/sequenced group charts dep Jun 23, 2017
@gardlt gardlt requested a review from alanmeadows June 23, 2017 21:48
@gardlt gardlt force-pushed the feat/apply/sequenced-group-charts-dep branch 3 times, most recently from 1d93c40 to 973195d Compare June 28, 2017 17:27
if self.wait:
if self.timeout:
chart_timeout = self.timeout
elif getattr(chart, 'timeout', None):
Copy link
Contributor

Choose a reason for hiding this comment

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

When would this path ever get activated -- a chart specific timeout? self.timeout has an oslo config default of 3600 so it would always be set?

Shouldn't this be:

if getattr(chart...)
  chart_timeout = chart.timeout
else
  chart_timeout = self.timeout

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 agree.

Copy link
Contributor

@alanmeadows alanmeadows left a comment

Choose a reason for hiding this comment

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

Alex, this looks good to me - one minor nit.

@gardlt gardlt force-pushed the feat/apply/sequenced-group-charts-dep branch from 973195d to 98da9fa Compare June 29, 2017 14:57
@gardlt gardlt requested review from drewwalters96 and a user June 29, 2017 15:05
@gardlt gardlt force-pushed the feat/apply/sequenced-group-charts-dep branch from 98da9fa to 8b0c727 Compare June 29, 2017 15:55
@gardlt gardlt self-assigned this Jun 29, 2017
@gardlt gardlt added this to the 0.5.0 milestone Jun 29, 2017
* added new structure to the armada yaml
@gardlt gardlt force-pushed the feat/apply/sequenced-group-charts-dep branch from 8b0c727 to 55c3a0f Compare June 30, 2017 12:40
@mark-burnett
Copy link
Contributor

This looks like a fine way to organize chart groups.

I would really like to see a "group wait" functionality in the future -- I'll make an issue.

@mark-burnett mark-burnett merged commit 35249aa into att-comdev:master Jun 30, 2017
gardlt pushed a commit to gardlt/armada that referenced this pull request Jul 3, 2017
…up-charts-dep

[Feat] apply/sequenced group charts dep
@gardlt gardlt deleted the feat/apply/sequenced-group-charts-dep branch July 26, 2017 00:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: dependecies - wait dep chats to be complete
3 participants