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

Maintenance: Updating Azure API versions to latest #987

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Tom-Sloboda
Copy link
Contributor

@Tom-Sloboda Tom-Sloboda commented Nov 4, 2022

Cleaning up dev code

This PR closes #

The changes in this PR are as follows:

  • Updating API versions

I have read the contributing guidelines and have completed the following:

  • Tested my code end-to-end against a live Azure subscription.
  • Updated the documentation in the docs folder for the affected changes.
  • Written unit tests against the modified code that I have made.
  • Updated the release notes with a new entry for this PR.
  • Checked the coding standards outlined in the contributions guide and ensured my code adheres to them.

If I haven't completed any of the tasks above, I include the reasons why here:

  • The PR does not include any new/modified code other than API version changes, so no unit tests are required

@ninjarobot
Copy link
Collaborator

We normally update these when in need of a new feature from an API, as sometimes new API versions ship with breaking changes to behaviors. Are there certain API versions you need updated to get a new feature or is this just a general update to latest versions?

@martinbryant
Copy link
Contributor

@isaacabraham can you have a look at reviewing this please?

@isaacabraham
Copy link
Member

The PR is so old now, the main codebase has moved on. It needs to be brought up (merged in) and then end-to-end tested before it can be accepted, I'm afraid.

I know this isn't great - it should have been addressed ages ago. It's partly because it didn't solve a specific issue that had been raised that we didn't look at it.

@isaacabraham
Copy link
Member

Just to add to that - if there are no specific issues / requirements that this PR fixes, I'm inclined to close it - changing versions like this does present a (small) risk.

@ninjarobot
Copy link
Collaborator

They are usually safe, and especially in the last 2 years the ARM team has really cracked down on breaking changes. But I've definitely run into unexpected issues where a resource changes default behavior in newer API versions. One that got our team was that by switching the API version, the default zone behavior changed (older version didn't have zone support) and this led to unexpected deployment errors.

I'm generally in favor of using the newest versions, just usually when someone does them in a PR, they are actively testing with that resource. I'd like us to make sure we are testing any version we upgrade with an actual ARM deployment and making sure we end up with the expected resources.

That said, some versions are old enough to be deprecated, so we may need to embark on this effort in a coordinated manner, or at least guide contributors to use the latest versions in the PR's so we are moving in that direction. I can think of three resources - classic AppInsights, single-server Postgres, and basic public IPs - that are very near deprecation as both an API and a resource.

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.

4 participants