Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

Only converge on wing when not in infrastructure only #493

Merged
merged 8 commits into from Nov 20, 2018
Merged

Only converge on wing when not in infrastructure only #493

merged 8 commits into from Nov 20, 2018

Conversation

JoshVanL
Copy link
Contributor

@JoshVanL JoshVanL commented Sep 6, 2018

What this PR does / why we need it:
Don't wait for wing to converge when we're not in infrastructure only

fixes #475

Don't wait for wing conversion when we're not in infrastructure only

@jetstack-bot jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 6, 2018
@JoshVanL JoshVanL force-pushed the 475-dont-converge-on-infrastructure branch from 6fbf148 to 5a944ae Compare September 6, 2018 09:07
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Sep 6, 2018
@JoshVanL JoshVanL force-pushed the 475-dont-converge-on-infrastructure branch from 5a944ae to f4d50e3 Compare September 6, 2018 09:29
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2018
@JoshVanL JoshVanL force-pushed the 475-dont-converge-on-infrastructure branch from f4d50e3 to 6b5d884 Compare September 17, 2018 08:04
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2018
@simonswine
Copy link
Contributor

Can you look at this together with our notes from the tarmak call last week:

@simonswine
Copy link
Contributor

/assign @JoshVanL

@simonswine simonswine added this to the release-0.5 milestone Sep 24, 2018
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2018
@JoshVanL JoshVanL force-pushed the 475-dont-converge-on-infrastructure branch from 84f306c to 60727d0 Compare September 27, 2018 11:51
@jetstack-bot jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 27, 2018
@JoshVanL
Copy link
Contributor Author

JoshVanL commented Oct 2, 2018

/test verify quick
/test verify docs

@JoshVanL
Copy link
Contributor Author

JoshVanL commented Oct 2, 2018

/test verify quick

@JoshVanL JoshVanL force-pushed the 475-dont-converge-on-infrastructure branch 2 times, most recently from 384836c to 653e727 Compare October 4, 2018 08:53
@JoshVanL
Copy link
Contributor Author

JoshVanL commented Oct 4, 2018

/unassign
/assign @simonswine

@jetstack-bot jetstack-bot assigned simonswine and unassigned JoshVanL Oct 4, 2018
MattiasGees
MattiasGees suggested changes Oct 18, 2018
View reviewed changes
Copy link
Member

@MattiasGees MattiasGees left a comment

Choose a reason for hiding this comment

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

Tested the behaviour and works. Code looks good to me. Only small docs change needed.

cmd/tarmak/cmd/cluster.go Show resolved Hide resolved
@MattiasGees
Copy link
Member

/unassign @simonswine
/assign @JoshVanL

@jetstack-bot jetstack-bot assigned JoshVanL and unassigned simonswine Oct 18, 2018
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2018
@MattiasGees
Copy link
Member

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2018
@JoshVanL JoshVanL force-pushed the 475-dont-converge-on-infrastructure branch from a5efc08 to e36feda Compare November 8, 2018 20:17
@jetstack-bot jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2018
@simonswine
Copy link
Contributor

I don't think that handles cluster apply -C correctly. I think in those cases it needs to be waiting for convergence as the configuration might have changed. (see my code commit)

/assign @JoshVanL
/unassign

@jetstack-bot jetstack-bot assigned JoshVanL and unassigned simonswine Nov 9, 2018
@simonswine
Copy link
Contributor

/unassign @MattiasGees

@JoshVanL
Only converge on wing when not in infrastructure only
e4e3fd7 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Adds wait for convergence flag
b657d90 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Only converge when there has been changes
8dd727e 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Fix wait for convergence api json header
e81fe31 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Uses variable when returning
d549805 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Update API reference docs
13e2821 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Adds wait-for-convergence to cli docs
6623c09 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL
Assume apply will change to default waiting for convergence
432b560 
Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>
@JoshVanL JoshVanL force-pushed the 475-dont-converge-on-infrastructure branch from e36feda to 432b560 Compare November 9, 2018 15:25
@JoshVanL
Copy link
Contributor Author

JoshVanL commented Nov 9, 2018

I have changed it so we assume a change will happen by default. This means we will wait for a convergence if we have not changed the infrastructure.

/unassign
/assign @simonswine

@jetstack-bot jetstack-bot assigned simonswine and unassigned JoshVanL Nov 9, 2018
@simonswine
Copy link
Contributor

Thank you that seems to work

/approve
/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2018
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MattiasGees, simonswine

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2018
@jetstack-bot jetstack-bot merged commit 7425d1c into jetstack:master Nov 20, 2018
@alljames alljames mentioned this pull request Nov 23, 2018
Closed
@dippynark dippynark mentioned this pull request Mar 5, 2019
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@simonswine simonswine simonswine left review comments

@MattiasGees MattiasGees MattiasGees requested changes

Assignees

@simonswine simonswine

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v0.5
Development

Successfully merging this pull request may close these issues.

Don't wait for wing to converge when running apply with --infrastructure only
4 participants
@JoshVanL @simonswine @MattiasGees @jetstack-bot