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

Update default Getters with known defaults #363

Merged
merged 1 commit into from May 23, 2022
Merged

Conversation

nywilken
Copy link
Member

  • By default XTerrformGet header support should be disable; clients
    needing supporting this header should overwrite the default getters
    with a custom HttpGetter

  • Add default timeouts for HTTP Read Requests; including HEAD requests

* By default XTerrformGet header support should be disable; clients
  needing supporting this header should overwrite the default getters
  with a custom HttpGetter

* Add default timeouts for HTTP Read Requests; including HEAD requests
Netrc: true,
Netrc: true,
XTerraformGetDisabled: true,
HeadFirstTimeout: 10 * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 Maybe we should go with DoNotCheckHeadFirst, to avoid the initial HEAD request altogether.

Suggested change
HeadFirstTimeout: 10 * time.Second,
DoNotCheckHeadFirst: true,

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Packer uses the default Getter settings with a few additional Getters. I updated them to match what we plan to use in Packer. That said, I think Packer could take advantage of the Range header. So I opted for the HeadFirstTimeout over the disabling it entirely. Does that sound reasonable to you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Given the need of the changes. I went ahead and merged as is. If after working through Packer further we find we can disable entirely. We can revisit. But sticking with this default for now.

Copy link
Member

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

👍🏼 LGTM!

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

Successfully merging this pull request may close these issues.

None yet

3 participants