Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Check stack existence via DescribeStacks #1480

Merged
merged 3 commits into from
Nov 5, 2018
Merged

Check stack existence via DescribeStacks #1480

merged 3 commits into from
Nov 5, 2018

Conversation

cknowles
Copy link
Contributor

@cknowles cknowles commented Nov 5, 2018

Rather than ListStacks without any paging.

Fixes #1479

@c-knowles
Check stack existence via DescribeStacks
0d67804 
Rather than ListStacks without any paging.

Fixes kubernetes-retired#1479
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 5, 2018
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 5, 2018
@c-knowles
Remove unused ListStacks
5ab1f0d 
Fix up tests to use the newly used DescribeStacks
@cknowles cknowles requested review from mumoshu and removed request for redbaron and danielfm November 5, 2018 09:15
@cknowles
Copy link
Contributor Author

cknowles commented Nov 5, 2018

@mumoshu do you know why I cannot request a review from @davidmccormick? I cannot get his user to appear on the reviewers list.

@codecov-io
Copy link

Codecov Report

Merging #1480 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1480      +/-   ##
==========================================
+ Coverage   37.91%   37.92%   +0.01%     
==========================================
  Files          75       75              
  Lines        4595     4596       +1     
==========================================
+ Hits         1742     1743       +1     
  Misses       2611     2611              
  Partials      242      242
Impacted Files Coverage Δ
cfnstack/cfnstack.go 85.96% <100%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2304b62...beae9b7. Read the comment docs.

@mumoshu
Copy link
Contributor

mumoshu commented Nov 5, 2018

@c-knowles Thanks for the fix!

do you know why I cannot request a review from @davidmccormick? I cannot get his user to appear on the reviewers list.

Just invited @davidmccormick as a collaborator w/ read/write privs to this repo, hoping it makes difference.

@davidmccormick
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2018
davidmccormick
davidmccormick approved these changes Nov 5, 2018
View reviewed changes
Copy link
Contributor

@davidmccormick davidmccormick left a comment

Choose a reason for hiding this comment

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

LGTM although given the number of modifications to the tests I'm half wondering whether we should implement some sort of DoesStackExist(cfi CFInterrogator, name string) (bool, error) to cut it down perhaps. This change should definitely be released on both v0.11.x and v0.12.x branches!

@davidmccormick davidmccormick removed their assignment Nov 5, 2018
@davidmccormick
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidmccormick

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2018
@k8s-ci-robot k8s-ci-robot merged commit bfa9f54 into kubernetes-retired:master Nov 5, 2018
@cknowles
Copy link
Contributor Author

cknowles commented Nov 6, 2018

@davidmccormick we could implement that although then it does not mirror the SDK so not sure if it's worth it.

@cknowles cknowles deleted the bug/fix-cf-stack-paging branch November 6, 2018 01:30
cknowles pushed a commit to cknowles/kube-aws that referenced this pull request Nov 6, 2018
@k8s-ci-robot @c-knowles
Merge pull request kubernetes-retired#1480 from c-knowles/bug/fix-cf-…
416ae12 
…stack-paging

Check stack existence via DescribeStacks
cknowles pushed a commit to cknowles/kube-aws that referenced this pull request Nov 6, 2018
@k8s-ci-robot @c-knowles
Merge pull request kubernetes-retired#1480 from c-knowles/bug/fix-cf-…
0ca4ed9 
…stack-paging

Check stack existence via DescribeStacks
@mumoshu mumoshu mentioned this pull request Nov 8, 2018
Closed
8 tasks
@kevtaylor kevtaylor mentioned this pull request Jan 15, 2019
Closed
@davidmccormick davidmccormick added this to the v0.13.0 milestone Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidmccormick davidmccormick davidmccormick approved these changes

@mumoshu mumoshu Awaiting requested review from mumoshu

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@cknowles @codecov-io @mumoshu @davidmccormick @k8s-ci-robot @c-knowles