[FeatureRequest: Issue #305] Support colon characters in field names #306
Conversation
In the ConvertToTerraformCompliantName func
Before first check for returning, otherwise a snakeCase string with colons will be returned when it is not actually a compliant name
The ConvertToTerraformCompliantName function cannot handle multiple number characters in a name.
|
Hi @bradthebuilder , thanks for opening the PR! I just had a baby so most likely won't have time to look into this for a little while. I am expecting to come back to work in couple weeks. I will review the PR and provide feedback then. Sorry for the delay! |
|
Hey @bradthebuilder , Thanks for contributing! The PR looks good, I went ahead and merged the new commits from master to bring this PR up to date. I see the CI build job is failing and I am guessing it's due to the build job being triggered from a fork and the docker login complaining about not being able to perform an interactive login. I was trying to push a new commit against your upstream Alternatively I can create a new PR against a new branch I pushed which contains the latest from master, your commits and the new commit that I wanted to add, you can check out the branch here: https://github.com/dikhan/terraform-provider-openapi/commits/bradthebuilder-master Let me know |
|
Howdy @dikhan , I admit, I'm a bit rusty with GitHub permissions because I do most of my work on Gitlab, but I just tried inviting you as a Collaborator to my fork; maybe accepting that invitation will allow you to push against my I did make make another commit on my feature/terraform-gte-1.0.0 branch that adjusts |
dikhan
force-pushed
the
master
branch
5 times, most recently
from
2500be6 to
e945e54
Compare
|
Hey @bradthebuilder , I managed to push to your fork in the end but unfortunately was not able to fix the issue with the builds not working in forks. Looks like in Travis CI encrypted environment variables are not available to pull requests from forks due to the security risk of exposing such information to unknown code, and hence commands like docker login used to be able to run the integration tests in the pipeline fail to execute from forks. Considering the build was failing due to reasons not related to this PR and also having confirmed that all the tests passed locally I feel confortable merging the PR even if the build is red. Once the PR is merged, all the tests will kick off again and this time they will execute properly since the build would be triggered from this repo itself. For the record I am attaching the output of the tests results run locally: PR306_test_results_run_locally.txt Lastly, @bradthebuilder I just wanted to let you know that the Publishing OpenAPI Terraform providers in the Terraform Registry doc has instructions on how to leverage the OpenAPI Terraform repo as a library and be able to publish your own Terraform provider (powered by the OpenAPI Terraform provider) in the Terraform registry in case you are interested. With that, thanks again for contributing to the repo! Dani |
commit 536983e Merge: d54067c 9bb8276 Author: Daniel I. Khan Ramiro <di.khan.r@gmail.com> Date: Fri Sep 3 09:09:56 2021 -0700 Merge pull request dikhan#310 from dikhan/release-v2.2.0 [NewRelease] v2.2.0 commit 9bb8276 Author: dikhan <di.khan.r@gmail.com> Date: Fri Sep 3 09:01:49 2021 -0700 Release v2.2.0 commit d54067c Merge: 382dda8 0f63ea1 Author: Daniel I. Khan Ramiro <di.khan.r@gmail.com> Date: Fri Sep 3 08:49:30 2021 -0700 Merge pull request dikhan#309 from dikhan/integrate-terraform-sdk-v2.7.1 [TechDebt: Issue dikhan#308] Integrate Terraform SDK 2.7.1 commit 0f63ea1 Author: dikhan <di.khan.r@gmail.com> Date: Wed Sep 1 19:00:55 2021 -0700 Integrate Terraform SDK 2.7.1 commit 382dda8 Merge: f4718a1 e945e54 Author: Daniel I. Khan Ramiro <di.khan.r@gmail.com> Date: Wed Jul 21 22:22:33 2021 -0700 Merge pull request dikhan#306 from bradthebuilder/master [FeatureRequest: Issue dikhan#305] Support colon characters in field names commit e945e54 Merge: 2067311 f4718a1 Author: Daniel I. Khan Ramiro <di.khan.r@gmail.com> Date: Mon Jul 19 16:06:53 2021 -0700 Merge branch 'master' into master commit f4718a1 Merge: 6d8b909 21f5806 Author: Daniel I. Khan Ramiro <di.khan.r@gmail.com> Date: Thu Jul 15 11:19:40 2021 -0700 Merge pull request dikhan#307 from dikhan/release-v2.1.0 [NewRelease] v2.1.0 commit 21f5806 Author: dikhan <di.khan.r@gmail.com> Date: Thu Jul 15 11:11:23 2021 -0700 Releasing v2.1.0 commit 6d8b909 Merge: 5a7c1e3 ec9d88c Author: Daniel I. Khan Ramiro <di.khan.r@gmail.com> Date: Thu Jul 15 07:59:05 2021 -0700 Merge pull request dikhan#295 from dikhan/feature/support-for-post-input-model-without-id-2 [FeatureRequest: Issue dikhan#100] Add support for POST with request schema only containing inputs (required/optional) and response only containing outputs (computed properties) commit ec9d88c Author: dikhan <di.khan.r@gmail.com> Date: Wed Jul 14 11:29:41 2021 -0700 go mod dep updates commit 237d338 Author: dikhan <di.khan.r@gmail.com> Date: Wed Jul 14 11:29:33 2021 -0700 Fix monitor int tests assertions that before were actually resolving to an actual domain - The monitor int tests rely on the error output to confirm the API call was made against the right reginal endpoint commit f0f7cf4 Author: Daniel Isaac Khan Ramiro <di.khan.r@gmail.com> Date: Mon Jul 12 21:54:55 2021 -0700 Add more test coverage commit c4f84e5 Author: Daniel Isaac Khan Ramiro <di.khan.r@gmail.com> Date: Mon Jul 12 21:45:59 2021 -0700 Remove erorr return from isResourceInstanceEndPoint signature - The function was never returning any error and therefore consumers of the method would check for an error for no reason. commit a39b4c1 Author: Daniel Isaac Khan Ramiro <di.khan.r@gmail.com> Date: Mon Jun 14 21:30:13 2021 -0700 Add more test coverage for validateRootPath method commit 9252e2f Author: Daniel Isaac Khan Ramiro <di.khan.r@gmail.com> Date: Wed Feb 10 19:19:33 2021 -0800 Add support for POST with request schema only containing inputs and response model containing computed properties
What problem does this Pull Request solve?
Please link to the issue number here (issue will be closed when PR is merged): Closes #305
Type of change
What type of change does your code introduce to the provider? Please put an x (w/o heading/trailing white spaces) in the boxes that apply:
Checklist
Please put an
x(w/o heading/trailing white spaces) in the boxes that apply:make test-all[FeatureRequest: Issue #X] <PR Title>[BugFix: Issue #X] <PR Title>[TechDebt: Issue #X] <PR Title>[NewRelease] vX.Y.ZChecklist for Admins
Commentary
All unit tests pass on my system.
The following integration tests in
tests/integration/resource_monitors_test.gofailed on my laptop before and after my changes were added:Integration test error stdout
I figured it had something to do with my local network's DNS not being configured to resolve the API URLs used in the tests (I guess the docker containers bridged to my laptop), so I figured Travis CI would be the gatekeeper in this discrepancy. The errors were present on the master branch before my additions and are still there afterward, so I'm confident my additions at least didn't make things worse. If this is an actual bug and not an issue with my environment, I can work on a separate bug-hunting branch.
The provider does compile and I tested it against the docker-compose local-env and my Antsle device. It works on my Antsle device perfectly, and 3/4 resources get created with the swaggercodegen example on
terraform apply(terraform validateandterraform planwork fine).I think it's because the resource name has more than one number character in it; I noticed when adding my unit tests that the
ConvertToTerraformCompliantNamefunction cannot handle more than one distinct number in a field name. Once again, I can work on this in a separate bug branch if we're in agreement.