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

[Linter] Update files with respect to sort-imports rule #18567

Closed
wants to merge 5 commits into from
Closed

[Linter] Update files with respect to sort-imports rule #18567

wants to merge 5 commits into from

Conversation

bzhang0
Copy link
Contributor

@bzhang0 bzhang0 commented Nov 8, 2021

This solves #9252. I've added the sort-imports rule into the ./common/tools/eslint-plugin-azure-sdk/.eslintrc.json and updated files appropriately after calling rush rebuild and rush lint.

I used the default settings for sort-imports, so if there should be changes to the rule settings, please let me know!

@ghost ghost added eslint plugin customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Nov 8, 2021
@ghost
Copy link

ghost commented Nov 8, 2021

Thank you for your contribution bzhang0! We will review the pull request and get back to you soon.

@bzhang0
Copy link
Contributor Author

bzhang0 commented Nov 8, 2021

Apologies, it seems like a different pull request #18565 is linked here as well. Perhaps this is because I solved the first issue in main instead of creating a new branch? Please excuse this, but if you know how to prevent this, please let me know so I can use that in the future! (I assume it is just making a new branch for every different issue).

@maorleger
Copy link
Member

maorleger commented Nov 8, 2021

Hi @bzhang0 - me again!

Yes, looks like your main branch contained some commits that are not part of this PR.

What I usually do is I keep my main branch empty of any changes. When I want to make a change I'll always make it in a branch. This way I can branch off of main at any point without anything sneaking in 😄

It looks like #18565 is based off of main and you have an open PR against so you don't want to blow those changes away yet. Here's what you can do instead:

  1. Create a new branch off of main
  2. in the new branch revert any commits that should not be there
  3. Cherry-pick or redo the commits that only matter for this changeset
  4. Open a new PR

Edit note: Changed instructions to avoid git reset and use git revert instead

There are other ways to tackle this if you don't want to redo changes - you can use git revert for example in this branch. I worry about merge-conflicts in this case since both commits apply to the same area of the codebase but if you can resolve those it could be easier.

If you have any questions, I'm happy to help - feel free to ping me!

@bzhang0
Copy link
Contributor Author

bzhang0 commented Nov 8, 2021

Hey @maorleger! Thank you for the feedback :)

I found out after I made my first pull request that I probably should have created a branch for each issue - I'm relatively new to using GitHub but always happy to learn :P.

Would it be okay to just delete my current fork and restart (creating two new branches for this and the previous PR)? I haven't made any changes that would take too long to fix if I started anew, I'm just not sure if there are any negative effects from deleting a fork. I feel like if I restart from a new fork, I can be more consistent in my PRs, changes, etc. Let me know if this is possible!

@maorleger
Copy link
Member

It'll be just fine with me 😄 I'm happy to review any new PRs you redo.

But, if I may make a recommendation - you don't need to delete your fork and fork again. If you wanted a nice clean slate you could the following:

git checkout main
git remote add upstream https://github.com/Azure/azure-sdk-for-js
git fetch upstream
git reset --hard upstream/main # Warning - destructive operation!
git push --force # Warning - destructive _github_ operation :) 

With all that said, feel free to do whatever is easier for you! I believe you can just delete and recreate your fork and it should be fine. Either way you will need to close these PRs and open new ones whenever you're ready.

Enjoy shaving yaks - we all have to do it!

@bzhang0
Copy link
Contributor Author

bzhang0 commented Nov 9, 2021

I'm having some issues following the method you suggested - for simplicities sake I think I will re fork and redo my fixes to this issue and #18565.

I will close this PR now, thank you for the help!

@bzhang0 bzhang0 closed this Nov 9, 2021
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-js that referenced this pull request Apr 14, 2022
Review request for Microsoft.ContainerService to add version 2022-03-02-preview (Azure#18633)

* Adds base for updating Microsoft.ContainerService from version preview/2022-02-02-preview to version 2022-03-02-preview

* Updates readme

* Updates API version in new specs and examples

* feat: add `/rotateServiceAccountSigningKeys` API (Azure#18359)

* feat: add `/rotateServiceAccountSigningKeys` API

* chore: remove duplicated description

* feat: add `workloadIdentity` settings to `SecurityProfile` (Azure#18360)

* fix: update example name (Azure#18382)

* add creationData to mc data (Azure#18414)

* add creationData to mc data

* fix test

* fix format

* Add API properties and example JSON for Web App Routing of IngressProfile (Azure#18564)

* Add API properties and example JSON for Web App Routing of IngressProfile.

* Add a ending period for description to match the style in all other "descriptions" in the same file.

* Update readmes for the 2022-03-02-preview dev branch of container service (Azure#18358)

* update readme

* update sdk readmes

* Agentpool alias minor version 2022-03-02-preview (Azure#18381)

* Add field currentOrchestratorVersion to support Agentpool alias minor version

* Add new exmaple for alias minor version

* Remove example for PrettierCheck

* Fix typo

* Latest patch version supported is 1.22.7 at the moment

* Address comments - refine descriptions for fields

* feat: add ManagedCluster StorageProfile in 0302preview (Azure#18590)

Signed-off-by: Ji An Liu <jiliu8@microsoft.com>

* Swagger change for ignore-pod-disruption-budget (Azure#18548)

* Swagger change for ignore-pod-disruption-budget

* Change ignorePodDisruptionBudget to string in example file.

* Change ignorePodDisruptionBudget to boolean type.

* add effectiveNoProxy for AKS (Azure#18544)

* add effectiveNoProxy for AKS

* add to correct api

* fix lowercase -> uppercase O

* spelling

* Replace common type definitions with references since 2022-03-02-preview for Microsoft.ContainerService  (Azure#18567)

* replace Resource

* replace SystemData

* replace parameters

* change track2 to python

Co-authored-by: hbc <bcxxxxxx@gmail.com>
Co-authored-by: Qingqing <qizhe@microsoft.com>
Co-authored-by: Yi Zhang <yizhang5@microsoft.com>
Co-authored-by: Thalia Wang <58485997+wenxuan0923@users.noreply.github.com>
Co-authored-by: Ji'an Liu <jiliu8@microsoft.com>
Co-authored-by: Tong Chen <45081443+tonche@users.noreply.github.com>
Co-authored-by: Ace Eldeib <aleldeib@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization. eslint plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants