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 messaging, it is used in the backend also #31

Merged
merged 3 commits into from
Jun 3, 2020
Merged

Conversation

paultyng
Copy link
Contributor

@paultyng paultyng commented Jun 1, 2020

Related to hashicorp/terraform-provider-aws#13057, its unclear, since this is a common depenedency, whether this is for a provider or backend, when it could be both. Modifying the messaging here to make that a little more obvious. Ideally we should just make downstream consumers wrap these errors.

@bflad
Copy link
Contributor

bflad commented Jun 1, 2020

Related issues:

I think we should completely remove mentions of which upstream caller might be involved (since it leaves it ambiguous to the operator to figure out which is applicable), instead allowing the S3 Backend/AWS Provider to return the proper error messaging context themselves.

e.g. In Terraform S3 Backend:

sess, err := awsbase.GetSession(cfg)

if err != nil {
  return fmt.Errorf("error configuring S3 Backend: %w", err)
}

e.g. in Terraform AWS Provider:

sess, accountID, partition, err := awsbase.GetSessionWithAccountIDAndPartition(awsbaseConfig)

if err != nil {
	return nil, fmt.Errorf("error configuring AWS Provider: %w", err)
}

@bflad
Copy link
Contributor

bflad commented Jun 1, 2020

Regarding the error message with the URL, that should be promoted to an exported type so the upstream code can catch that specific case and create their own appropriate messaging if they desire.

@bflad bflad linked an issue Jun 2, 2020 that may be closed by this pull request
@bflad bflad added the bug Something isn't working label Jun 3, 2020
@bflad bflad added this to the v0.5.0 milestone Jun 3, 2020
@bflad bflad self-assigned this Jun 3, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Going to pull this in as-is for now and iterate on it (likely along with #25). 👍 Thanks, @paultyng!

@bflad bflad merged commit a5d46a2 into master Jun 3, 2020
@bflad bflad deleted the paultyng-patch-1 branch June 3, 2020 00:49
bflad added a commit that referenced this pull request Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants