Skip to content

[Automation] New SDK for 2018-06-30 changes and other nit fixes#4916

Closed
jemex wants to merge 1 commit intoAzure:psSdkJson6from
D1v38om83r:jeabdu
Closed

[Automation] New SDK for 2018-06-30 changes and other nit fixes#4916
jemex wants to merge 1 commit intoAzure:psSdkJson6from
D1v38om83r:jeabdu

Conversation

@jemex
Copy link
Copy Markdown
Contributor

@jemex jemex commented Oct 23, 2018

Description

  1. Python2Packages for azure automation. Added new API specs, re-recorded tests for Automation scenarios. Added a new test for Python2Package scenarios.
    API PR: Created specs for python 2 packages azure-rest-api-specs#3713

  2. Dynamic Group support change
    API PR : Swagger change for Update configuration dynamic group azure-rest-api-specs#4165

  3. Pre- post support change
    API PR : Swagger change for pre and post script azure-rest-api-specs#4298


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

@dsgouda
Copy link
Copy Markdown
Contributor

dsgouda commented Oct 24, 2018

  1. Please provide a meaningful title for the PR
  2. Looks like you are bumping the REST API version, this would require major version bumps for the nuget package in csproj and AssemblyInfo.cs files

@jemex jemex changed the title Jeabdu [Automation] New SDK for 2018-06-30 changes and other nit fixes Oct 24, 2018
@jemex
Copy link
Copy Markdown
Contributor Author

jemex commented Oct 25, 2018

@dsgouda - Thanks for the catch. I've updated the files you mentioned. Could you please take a look?

@vrdmr FYI.

Copy link
Copy Markdown
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

@jemex this looks like a stable API version. We recommend publishing stable Nuget packages for stable API versions and preview Nuget packages for preview API versions.
Please consider publishing a stable Nuget version here.
Also, please squash commits into a single commit (this may require recreating the branch with these changes)

Copy link
Copy Markdown
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Why was tools/PS-VSPrompt.lnk added?
Please remove this file if not required

@D1v38om83r
Copy link
Copy Markdown
Contributor

@dsgouda can't we just squash the commits while merging? its way more efficient than having to rebase.

@dsgouda
Copy link
Copy Markdown
Contributor

dsgouda commented Oct 25, 2018

If changes from a remote have been merged the squash is not very effective

@D1v38om83r
Copy link
Copy Markdown
Contributor

changes have been squashed and changes to tools/PS-VSPrompt.lnk have been reverted

@dsgouda
Copy link
Copy Markdown
Contributor

dsgouda commented Oct 25, 2018

@D1v38om83r Do you still plan to release a preview Nuget package?

@vrdmr
Copy link
Copy Markdown
Member

vrdmr commented Oct 25, 2018

@dsgouda - When you say

@D1v38om83r Do you still plan to release a preview Nuget package?

Do you mean to jump to 4.0.0 or 3.5.0 (without the -preview suffix)?

Also,

@jemex this looks like a stable API version. We recommend publishing stable Nuget packages for stable API versions and preview Nuget packages for preview API versions.
Please consider publishing a stable Nuget version here.
Also, please squash commits into a single commit (this may require recreating the branch with these changes)

The newest files are generated from the Tag: package-2018-06-preview, which contains -preview APIs as well. In this case, can we generate a stable SDK version?

Please let us know.

FYI @jemex @D1v38om83r

@jemex
Copy link
Copy Markdown
Contributor Author

jemex commented Oct 26, 2018

@dsgouda - can you please answer @vrdmr question? we are waiting to address the feedback and merge the change

@vrdmr
Copy link
Copy Markdown
Member

vrdmr commented Oct 29, 2018

@dsgouda Ping

@dsgouda
Copy link
Copy Markdown
Contributor

dsgouda commented Oct 29, 2018

@vrdmr @jemex Talked to @shahabhijeet and our recommendation is to change the version here to 3.8.0-preview. That way if you plan to publish some changes to the previous API version you'll still have bandwidth upto 3.7.0-preview to publish those changes and whenever you plan to release a stable version for the Nuget package you can simply start with 4.0.0.

@D1v38om83r
Copy link
Copy Markdown
Contributor

@dsgouda do we need to make any changes at our end after your commits?

Is the PR good to go? I have made the change you asked for "Why was tools/PS-VSPrompt.lnk added?
Please remove this file if not required" in this commit 5db0efc

@dsgouda
Copy link
Copy Markdown
Contributor

dsgouda commented Oct 29, 2018

@D1v38om83r like I mentioned please set the version to 3.8.0-preview in the csproj and in the AssemblyInfo.cs file
Also, squash the commits and this PR is good to merge

for Automation DynamicGroup and Prepost

updating release notes (#4926)

* updating release notes

* updating CR and CR.Azure version for all SDKs

Update common libraries with Newtonsoft Json 10 (#4932)

* versioning

* add package

* fix test reference

* add more packages

Newtonsoft (#4931)

* update version of newtonsoft json

* Update AzSdk.test.reference.props

* Update Sql.Tests.csproj

added new Boolean property KafkaEnabled to enable Kafka for the Event Hub namepsace (#4961)

Changed automation version to 3.8.0-preview
@D1v38om83r
Copy link
Copy Markdown
Contributor

Changed the versions and squashed the commits.

@dsgouda
Copy link
Copy Markdown
Contributor

dsgouda commented Oct 30, 2018

@jemex there are a lot of unrelated changes introduced, please sync the branch correctly with remote psSdkJson6, we can get on an offline chat if you need help

@jemex
Copy link
Copy Markdown
Contributor Author

jemex commented Oct 30, 2018

@dsgouda, if we had not squashed the commits before the final approval, we would have easier to fix this . now we have to go through 268 files to see what changes are unrelated. can you please help us on Friday or Monday to fix this?

@dsgouda
Copy link
Copy Markdown
Contributor

dsgouda commented Oct 30, 2018

There is a simple way to do it.

  1. Copy all the files you have modified in a temp location outside the repo
  2. Checkout psSdkJson6 branch, pull latest changes from remote (git pull https://github.com/Azure/azure-sdk-for-net.git psSdkJson6)
  3. Create a new branch (git checkout -b branch_name)
  4. Copy over the changes from temp location to this branch
  5. Push this branch to remote (git push upstream branch_name)
  6. Close this PR and open a new PR with the new branch and reference this PR there

Hope this helps

@D1v38om83r
Copy link
Copy Markdown
Contributor

Closing this PR, new PR is here #4971

@jemex
Copy link
Copy Markdown
Contributor Author

jemex commented Oct 30, 2018

Closing this PR, we have new PR in #4971

@jemex jemex closed this Oct 30, 2018
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