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

Support URNs properly #426

Merged
merged 3 commits into from
Apr 22, 2021
Merged

Conversation

buffer51
Copy link
Contributor

@buffer51 buffer51 commented Apr 21, 2021

Fixes #425 by refactoring the URL validation to support both URLs & URNs.

I looked for Go packages that do URN validation, and found voicera/gooseberry urn#TryParseString and leodido/go-urn#Parse. Neither seemed worth adding as a dependency, so I used the simple validation logic from the former.

@buffer51 buffer51 changed the title [WIP] Support URNs properly Support URNs properly Apr 21, 2021
@buffer51
Copy link
Contributor Author

I mistakenly had assumed URNs have 3 parts, but it's 3 or more parts. If we wanted to be stricter we could use a URN regex to make sure it's totally valid per the RFC (Google suggests a few), but we can't actually validate the parts (we'd need a library of known URN namespaces, which feels overkill).

Let me know what you think is more appropriate.

@manicminer
Copy link
Member

@buffer51 Thanks for updating. A simple string split is perfectly fine, we don't want to be overly strict and I don't think it's necessary to wield a regex 🙂

@buffer51
Copy link
Contributor Author

Brilliant @manicminer 😄 If I may, what is the release cadence for this project?

@manicminer
Copy link
Member

That's difficult to characterize as we're working through the Microsoft Graph transition at the moment, but there is hopefully a release forthcoming in the next week or so.

Copy link
Member

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

Thanks again @buffer51 and @MarkDordoy, this LGTM 👍

@manicminer manicminer merged commit e7fbf14 into hashicorp:main Apr 22, 2021
manicminer added a commit that referenced this pull request Apr 22, 2021
@ghost
Copy link

ghost commented May 20, 2021

This has been released in version 1.5.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azuread" {
    version = "~> 1.5.0"
}
# ... other configuration ...

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application identifier_uris no longer supports URNs and string values that are not considered a URL
2 participants