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

Upgrade Automation/Runbook API version from 2020-01-13-preview to 2019-06-01 #18213

Merged
merged 4 commits into from
Oct 21, 2022

Conversation

wuxu92
Copy link
Contributor

@wuxu92 wuxu92 commented Sep 1, 2022

For updating from 2020-01-13-preview to 2019-06-01 may cause incompatible changes, so I only updated the Runbook resource which is trying to fix the issue #1602. the version number of 2019-06-01 looks smaller than the 2020-01-13, but there is no newer stable version for runbook to use.

and because there are some breaking changes in swagger which cause the RunbookClient.GetContent and RunbookDraftClinet.Create cannot work in 2019-06-01 so these two Calls keep using the 2020-01-13-preview of Azure/azure-sdk-for-go. more detail in this issue comment: Azure/azure-sdk-for-go#17591 (comment) .

@wuxu92 wuxu92 changed the title Upgrade Automation/Runbook API version from 2020-01-13-preview to 2019-06-01 Upgrade Automation/Runbook API version from 2020-01-13-preview to 2019-06-01 Sep 1, 2022
@katbyte katbyte self-assigned this Sep 2, 2022
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

This also seems blocked by a bug in the swagger/generated SDK? we should not be rolling back to an old API version or have the service split across api versions

@wuxu92
Copy link
Contributor Author

wuxu92 commented Sep 7, 2022

yes, the root source is the swagger broken. en... as for runbook resource, it uses stable/2018-06-30/runbook.json in 2020-01-13-preview Track 1 sdk, so it is a kind of API upgrade in this PR in fact. the track 1 version management is a bit messy I think.

and hashicorp/go-azure-sdk cannot generate for a single resource making us have to cross API versions. or we directly generate the stable/2018-06-30 version in hashicorp/go-azure-sdk then we can fix the runbook issue all in one version. but still, that makes us keep both hashicorp and azure versions of sdk in azurerm.

@katbyte
Copy link
Collaborator

katbyte commented Sep 9, 2022

@wuxu92 - can we get the service team to fix the swagger? or is this the way they are going to detail their api in the specs going forward? if so it seems like what we should do is fix the pandora generator so it handles this new file type?

@wuxu92
Copy link
Contributor Author

wuxu92 commented Sep 9, 2022

yes, seems the service team will not revert this change because azure-sdk-go-go has released a Track2 SDK(which is compatible with this swagger) to replace the track1 SDK. As I know that @manicminer is working on the pandora generator to use a new base layer to fix this. but i don't know when will it be released.

@manicminer
Copy link
Contributor

@wuxu92 The new base layer is not too far from ready but is still going through testing. Can you detail the issue? If you think this should/could be solved with an improved base client I'd like to make sure that's indeed the case :)

@wuxu92
Copy link
Contributor Author

wuxu92 commented Sep 13, 2022

@manicminer Thanks for your work! The case here is when the response body is a single string, not structured data, Track1 and current pandora will try to json.Unmarshal the HTTP response body to a string pointer which raises a JSON error. it would be beneficial if you could test this case in the new base layer.

Below is an example of track1 code (and the current pandora SDK is almost the same).

func (client DscConfigurationClient) GetContentResponder(resp *http.Response) (result String, err error) {
	err = autorest.Respond(
		resp,
		azure.WithErrorUnlessStatusCode(http.StatusOK),
		autorest.ByUnmarshallingJSON(&result.Value),   // <-- result.Value is a *string
		autorest.ByClosing())

the error message: Error occurred unmarshalling JSON - Error = 'invalid character 'C' looking for beginning of value'

@katbyte
Copy link
Collaborator

katbyte commented Sep 13, 2022

@wuxu92 - how does track 2 handle it differently?

@wuxu92
Copy link
Contributor Author

wuxu92 commented Sep 13, 2022

@katbyte Track2 replaced autorest with a new base layer, but it has new authentication and other features designed not working for azurerm.

@katbyte
Copy link
Collaborator

katbyte commented Sep 14, 2022

@wuxu92 - i meant more how does it handle this "file" swagger change?

@katbyte katbyte added this to the Blocked milestone Sep 15, 2022
@tombuildsstuff
Copy link
Contributor

Blocked on Azure/azure-rest-api-specs#20588

@DuneganS
Copy link

Since Azure/azure-rest-api-specs#20588 was merged 3 days ago, is there anything blocking this going though?

@katbyte katbyte removed this from the Blocked milestone Sep 28, 2022
@katbyte
Copy link
Collaborator

katbyte commented Sep 28, 2022

@wuxu92 - now that the swagger PR has been merged can we update the go-azure-sdk version (in a separate PR please) if needed and not split the service across API versions now?

@wuxu92
Copy link
Contributor Author

wuxu92 commented Sep 28, 2022

@katbyte since the hashicorp/go-azure-sdk's new base layer is not released, and the current generated code for 2019-06-01 still broken the process(as described before). so it's still needed to use both hashi SDK and azure Trask1 SDK.

I think a better way is wanting for @manicminer 's new base layer to be ready, so we can use only hashi's SDK to support this. it is expected to be ready in recent weeks.

@DuneganS
Copy link

DuneganS commented Oct 4, 2022

I think a better way is wanting for @manicminer 's new base layer to be ready, so we can use only hashi's SDK to support this. it is expected to be ready in recent weeks.

Does this mean that this change would not resolve the issue? Im just looking for an ETA on when we will be able to create Python based runbooks using Terraform.

@katbyte
Copy link
Collaborator

katbyte commented Oct 12, 2022

@wuxu92 - ok, is this good to merge before then to add the new property? tests seem to be passing and we can consolidate versions at a later date

@wuxu92
Copy link
Contributor Author

wuxu92 commented Oct 19, 2022

@katbyte thanks for your review i have updated the pr and re-run the tests

image

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

thanks @wuxu92 - LGTM 🍄

@katbyte katbyte merged commit 927239a into hashicorp:main Oct 21, 2022
@github-actions github-actions bot added this to the v3.28.0 milestone Oct 21, 2022
@github-actions
Copy link

This functionality has been released in v3.28.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@DuneganS
Copy link

DuneganS commented Oct 25, 2022

Can anyone confirm that this was included in version 3.28.0? In the documentation I still do not see the options for Python2/3 and and I still get the error that they are not options.

@wuxu92
Copy link
Contributor Author

wuxu92 commented Oct 25, 2022

Can anyone confirm that this was included in version 3.28.0? In the documentation I still do not see the options for Python2/3 and and I still get the error that they are not options.

hi @DuneganS python 2/3 has been added in #18921 which release in 3.28.0 too.
could you please share the detail of the current error?

Python 2/3 should be supported in release 3.29 I think.

@wuxu92 wuxu92 deleted the fix/runbookPy branch October 25, 2022 05:28
@wuxu92
Copy link
Contributor Author

wuxu92 commented Oct 25, 2022

@katbyte looks like these two PRs with the wrong release version, they should be released in 3.29.0? 3.28.0 was released on (October 20, 2022) but these two merged on Oct. 22.

@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 Nov 25, 2022
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.

5 participants