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.

Sign up for GitHub

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

Jump to bottom

Zone Lockdown Implementation #115

Merged
merged 17 commits into from
Sep 19, 2018
Merged

Zone Lockdown Implementation #115

merged 17 commits into from
Sep 19, 2018

Conversation

SteveGoldthorpe-Work
Copy link

@SteveGoldthorpe-Work
zone_lockdown code - Squashed commit:
d3269a7 
commit d41eff9
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 15:43:07 2018 +0100

    make paused optional and default to false.

commit aa001d6
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 15:17:36 2018 +0100

    no need to set ID in update

commit 0235b67
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 13:47:44 2018 +0100

    add lockdown doc to erb

commit 99e9b8a
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 13:45:06 2018 +0100

    Add zone lockdown documentation.

commit 66e9555
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 13:20:43 2018 +0100

    Update was still based on AccessUpdate - fixed

commit fb782f4
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 10:49:20 2018 +0100

    need to set type for urls

commit 14cae09
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Fri Sep 7 17:26:33 2018 +0100

    1st version - import seems to work, update not yet
@SteveGoldthorpe-Work
add zone_lockdown documentation
64e014e
@SteveGoldthorpe-Work
Update cloudflare-go library to latest upstream version
e85312c
@ghost ghost added the size/L label Sep 10, 2018
@SteveGoldthorpe-Work
zone_lockdown code - Squashed commit:
7e5e994 
commit d41eff9
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 15:43:07 2018 +0100

    make paused optional and default to false.

commit aa001d6
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 15:17:36 2018 +0100

    no need to set ID in update

commit 0235b67
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 13:47:44 2018 +0100

    add lockdown doc to erb

commit 99e9b8a
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 13:45:06 2018 +0100

    Add zone lockdown documentation.

commit 66e9555
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 13:20:43 2018 +0100

    Update was still based on AccessUpdate - fixed

commit fb782f4
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 10:49:20 2018 +0100

    need to set type for urls

commit 14cae09
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Fri Sep 7 17:26:33 2018 +0100

    1st version - import seems to work, update not yet
@SteveGoldthorpe-Work
add zone_lockdown documentation
13c573b
@SteveGoldthorpe-Work
Update cloudflare-go library to latest upstream version
61ce968
@SteveGoldthorpe-Work
Merge branch 'sg-lockdown-prep' of github.com:SteveGoldthorpe-Work/te…
40dc150 
…rraform-provider-cloudflare into sg-lockdown-prep
@SteveGoldthorpe-Work SteveGoldthorpe-Work mentioned this pull request Sep 10, 2018
Closed
@SteveGoldthorpe-Work
change log to DEBUG
4dfc1f9
@SteveGoldthorpe-Work
remove zone_id from schema, more consistent naming
82d70c6
@SteveGoldthorpe-Work
fix doc about zone/zone_id.
45b395b
@SteveGoldthorpe-Work
zone_lockdown code - Squashed commit:
bdc4a41 
commit d41eff9
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 15:43:07 2018 +0100

    make paused optional and default to false.

commit aa001d6
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 15:17:36 2018 +0100

    no need to set ID in update

commit 0235b67
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 13:47:44 2018 +0100

    add lockdown doc to erb

commit 99e9b8a
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 13:45:06 2018 +0100

    Add zone lockdown documentation.

commit 66e9555
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 13:20:43 2018 +0100

    Update was still based on AccessUpdate - fixed

commit fb782f4
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Mon Sep 10 10:49:20 2018 +0100

    need to set type for urls

commit 14cae09
Author: Steve Goldthorpe - Work <25055153+SteveGoldthorpe-Work@users.noreply.github.com>
Date:   Fri Sep 7 17:26:33 2018 +0100

    1st version - import seems to work, update not yet
@SteveGoldthorpe-Work
add zone_lockdown documentation
434abd0
@SteveGoldthorpe-Work
change log to DEBUG
b97204f
@SteveGoldthorpe-Work
remove zone_id from schema, more consistent naming
b1b83a7
@SteveGoldthorpe-Work
fix doc about zone/zone_id.
1b76e32
@SteveGoldthorpe-Work
Merge branch 'sg-lockdown-prep' of github.com:SteveGoldthorpe-Work/te…
a18c645 
…rraform-provider-cloudflare into sg-lockdown-prep
@SteveGoldthorpe-Work
fix merge erb duplication
d32f286
@SteveGoldthorpe-Work SteveGoldthorpe-Work changed the title Zone Lockdown Implememtation Zone Lockdown Implementation Sep 13, 2018
@SteveGoldthorpe-Work SteveGoldthorpe-Work changed the base branch from master to stable-website September 13, 2018 12:17
@SteveGoldthorpe-Work SteveGoldthorpe-Work changed the base branch from stable-website to master September 13, 2018 12:17
@SteveGoldthorpe-Work
Copy link
Author

Reset base branch so PR only shows changes in this PR (and not all the changes made by re-basing from master).

@patryk
Copy link

patryk commented Sep 19, 2018 •
edited
Loading

Looks ok, but in general I would like to support zone_id in all resources (look at https://github.com/terraform-providers/terraform-provider-cloudflare/blob/d6fd35e74592630fb8a9ce5d8d90727d79f76560/cloudflare/resource_cloudflare_access_rule.go#L100-L105).

Our intention is to refactor all resources to take zone_id directly as we want to provide direct zone signup functionality in the near future.

@SteveGoldthorpe-Work
Copy link
Author

SteveGoldthorpe-Work commented Sep 19, 2018 •
edited
Loading

Hmm I originally had zone_id as a computed field but was asked to remove it in a previous review. Also isn't there a race condition if we always go with a zone_id and the zone (string) changes (admittedly this wouldn't happen often), how would we know to re-look up the zone_id. Should we just do that every time?

@patryk patryk merged commit dd4edc2 into cloudflare:master Sep 19, 2018
@patryk
Copy link

patryk commented Sep 19, 2018

I've coded access_rule to use zone_id exclusivity after the initial lookup in create function. In schema there's ForceNew which will delete/create a new resource when zone/zone_id change.

The reasoning is that user can be invited to many accounts and they can have zone signed up separately (you'll end up with many different zones having the same name, obviously only one of them will be active). Doing zone lookup every time would give nondeterministic results.

Merged this one, but will refactor a bit to include the intended behaviour.

I appreciate @jacobbednarz's review here, he didn't know what we intend to move away from zone name in favour of zone ID.

@SteveGoldthorpe-Work
Copy link
Author

Refactor - yes that's fine - hard to work in collaboration when some of the design goals aren't immediately obvious. Is there some forum, list or chat where things like this can be discussed? Might have a spare few cycles to help over the next few weeks.

@patryk
Copy link

patryk commented Sep 20, 2018

Good question, we'll try to figure something out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobbednarz jacobbednarz jacobbednarz left review comments

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SteveGoldthorpe-Work @patryk @jacobbednarz