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.

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

Added support for custom Directors #43

Merged
merged 1 commit into from
Sep 25, 2018
Merged

Conversation

hynd
Copy link
Contributor

@hynd hynd commented Sep 15, 2017

Hello!
Custom director's aren't particularly well documented on Fastly's site yet, but essential for anything other than per-request random balancing across origins.

thommahoney
thommahoney previously approved these changes Oct 19, 2017
@thommahoney
Copy link
Member

Sorry for the delayed response. Hoping to have an answer for you soon.

@hynd
Copy link
Contributor Author

hynd commented May 15, 2018

Hi @thommahoney - any progress on an answer for this? Thanks!

@lanej
Copy link
Contributor

lanej commented Sep 11, 2018

@hynd if you can resolve the conflicts, we can re-review this PR. Thanks for your patience.

@ghost ghost added the size/XL label Sep 11, 2018
@hynd
Copy link
Contributor Author

hynd commented Sep 11, 2018

Sure can! Rebased...

@@ -217,6 +250,15 @@ used in the `request_condition`, `response_condition`, or
* `priority` - (Optional) A number used to determine the order in which multiple
conditions execute. Lower numbers execute first. Default `10`.

The `director` block supports:

Choose a reason for hiding this comment

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

Should the director block support all the fields that the director api has available? https://docs.fastly.com/api/config#director

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be ideal but not required (cc/ @hynd). I would prefer to prioritize the relief of the users for features that can easily be augmented in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot - comment was missing from the doc, i think the only thing actually missing from the code is the shield field (which isn't in the underlying go-fastly lib.... yet)

Copy link
Contributor Author

@hynd hynd Sep 12, 2018

Choose a reason for hiding this comment

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

Ah, and capacity is only half-implemented in go-fastly - i'll PR fixes for both of those first, and will update this accordingly....

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @hynd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - merged upstream and refreshed this PR

lanej
lanej previously requested changes Sep 12, 2018
@@ -1350,7 +1405,7 @@ func resourceServiceV1Update(d *schema.ResourceData, meta interface{}) error {
Name: cf["name"].(string),
}

log.Printf("[DEBUG] Fastly Conditions Removal opts: %#v", opts)
log.Printf("[DEBUG] Condition Removal opts: %#v", opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

While I do agree that adding 'Fastly' to the log line is redundant, it has no relation to the intended change and should be done in another PR with a dedicated purpose.

The same is true for the remaining changes below.

@lanej lanej dismissed thommahoney’s stale review September 12, 2018 18:40

Stale review by ~8 months.

@ghost ghost added the size/XL label Sep 12, 2018
@ghost ghost added the size/XL label Sep 12, 2018
@ghost ghost added the size/XL label Sep 13, 2018
@lanej lanej dismissed their stale review September 20, 2018 17:29

Thanks for removing those message changes

@lanej
Copy link
Contributor

lanej commented Sep 20, 2018

Got an acceptance test failure:

--- FAIL: TestAccFastlyServiceV1_directors_basic (18.96s)
	testing.go:434: Step 0 error: Check failed: Check 2/4 error: Bad match Director match, expected (&fastly.Director{ServiceID:"3UIdDTdTSwAh1AZ1rw20RS", Version:1, Name:"mydirector", Comment:"", Shield:"", Quorum:0x0, Type:0x3, Retries:0x0, Capacity:0x0, DeletedAt:"", CreatedAt:"", UpdatedAt:""}), got (&fastly.Director{ServiceID:"3UIdDTdTSwAh1AZ1rw20RS", Version:1, Name:"mydirector", Comment:"", Shield:"", Quorum:0x4b, Type:0x3, Retries:0x5, Capacity:0x64, DeletedAt:"", CreatedAt:"2018-09-19T17:35:52Z", UpdatedAt:"2018-09-19T17:35:52Z"})

Looks like the timestamps don't match. I haven't dug into the reason but if you could investigate, I would appreciate it.

@ghost ghost added the size/XL label Sep 24, 2018
@hynd
Copy link
Contributor Author

hynd commented Sep 24, 2018

Fixed! Also removed the type "2" (round-robin) director, which seems to have been recently dropped from the docs (and throws an HTTP 400).

@ghost ghost added the size/XL label Sep 24, 2018
@ghost ghost added the size/XL label Sep 24, 2018
@ghost ghost added the size/XL label Sep 24, 2018
@ghost ghost added the size/XL label Sep 24, 2018
@lanej lanej merged commit d703504 into fastly:master Sep 25, 2018
@lanej
Copy link
Contributor

lanej commented Sep 25, 2018

Thanks @hynd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants