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.

Sign up for GitHub

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

Jump to bottom

chore: Add jsonfmt to CI #6496

Closed
wants to merge 1 commit into from
Closed

chore: Add jsonfmt to CI #6496

wants to merge 1 commit into from

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Jun 28, 2019 •
edited
Loading

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jun 28, 2019 •
edited
Loading

In Testing, Please Ignore

[Logs] (Generated from 1211262, Iteration 19)

No readme.md specification configuration files were found that are associated with the files modified in this pull request.

@AutorestCI
Copy link

AutorestCI commented Jun 28, 2019 •
edited
Loading

Automation for azure-sdk-for-go

Unable to detect any generation context from this PR.

@AutorestCI
Copy link

AutorestCI commented Jun 28, 2019 •
edited
Loading

Automation for azure-sdk-for-java

This PR contains more than 3 context, SDK generation is not enabled. Contexts found:

  • migrateprojects/resource-manager
  • alertsmanagement/resource-manager
  • monitor/resource-manager
  • cognitiveservices/data-plane/LUIS/Runtime
  • resources/resource-manager
  • sql/resource-manager

@AutorestCI
Copy link

AutorestCI commented Jun 28, 2019 •
edited
Loading

Automation for azure-sdk-for-ruby

This PR contains more than 3 context, SDK generation is not enabled. Contexts found:

  • hdinsight/resource-manager
  • security/resource-manager
  • reservations/resource-manager
  • containerservice/resource-manager
  • authorization/resource-manager
  • alertsmanagement/resource-manager
  • netapp/resource-manager
  • managementgroups/resource-manager
  • blueprint/resource-manager

@AutorestCI
Copy link

AutorestCI commented Jun 28, 2019 •
edited
Loading

Automation for azure-sdk-for-python

Unable to detect any generation context from this PR.

@azuresdkci azuresdkci requested a review from erich-wang June 28, 2019 16:17
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@nschonni
Copy link
Contributor Author

@sergey-shandar @jhendrixMSFT since most of the formatting PRs are landed, I figure it might be ok to introduce this again. I added a commit that finishes off the formatting changes, althought it is a cross a few APIs. Figure that doesn't matter since it is only whitespace changes, so generating the SDK shouldn't be needed

@nschonni nschonni force-pushed the setup-jsonfmt branch 3 times, most recently from d24c2ba to ce0fbcf Compare July 1, 2019 05:44
@erich-wang
Copy link
Member

@nschonni , could you please fix CI errors first?
@sergey-shandar @jhendrixMSFT, do you have any comments?

@erich-wang
Copy link
Member

one concern is that caarlos0/jsonfmt is maintained by individual and it may break

@jhendrixMSFT
Copy link
Member

To me it looks good however I'm not super-familiar with the CI pipeline so @sergey-shandar will have to review that part.

@nschonni
Copy link
Contributor Author

nschonni commented Jul 2, 2019 •
edited
Loading

@nschonni , could you please fix CI errors first?

I'll rebase again, but there is now #6529 #6530, #6531 and #6543, but non of them have model validation errors (it was just blueprint)

one concern is that caarlos0/jsonfmt is maintained by individual and it may break

Sure, maybe there is something else you'd prefer, but I found this was fast and pretty simple (code-wise). Looks like the author is also the maintainer for goreleaser, but I'm not sure if this tool is part of that large project or not.

This was referenced Jul 2, 2019
Closed
Closed
@nschonni
Copy link
Contributor Author

nschonni commented Jul 2, 2019

@erich-wang you can probably unselect the other reviews since they were added by CODEOWERS for files no longer included in the PR. Maybe add @sergey-shandar though

@AutorestCI AutorestCI mentioned this pull request Jul 3, 2019
Closed
@erich-wang
Copy link
Member

@nschonni , thank you for your contribution. Just discussed with @sergey-shandar, we suggested to remove jsonfmt job first from this PR because of legal issue.

@nschonni
Copy link
Contributor Author

nschonni commented Jul 3, 2019

Not sure if it understand what the legal issue is? I did notice that they didn't explictly add the MIT license in the repo, but only the metadata, so I opened an issue for that

@erich-wang
Copy link
Member

legal issue is one concern. Docker image maintained by individual is also unacceptable because:

  1. That kind of docker image is black box to us, we can't take the risk
  2. Individual may update/delete the docker image from hub without notification

@erich-wang
Copy link
Member

@nschonni , do you plan to create new PR or update this PR to remove jsonfmt job? I'll close this PR if it is the former.

@nschonni
Copy link
Contributor Author

nschonni commented Jul 9, 2019 •
edited
Loading

A license has now been added to the repo (I believe that covers your legal concern). I'll swap over to Snap on a Ubunutu image if you have concerns with Docker.
Edit: Snap couldn't access the directories, so switched to .deb

@nschonni nschonni force-pushed the setup-jsonfmt branch 7 times, most recently from 03109e1 to 3786ca7 Compare July 10, 2019 01:27
@nschonni nschonni requested a review from jaredmoo as a code owner July 10, 2019 01:37
@nschonni
Copy link
Contributor Author

Sent a separate PR for the model validation in #6576

@erich-wang
Copy link
Member

As I mentioned before, we're not going to depend on bits maintained by individual, which definitely introduces risk, .e.g somehow the individual delete the bits or update the bits with break change. So please remove the job for now, we may find out a feasible way later.

@nschonni
Copy link
Contributor Author

@erich-wang what is an acceptable tool for you then? The CI is the important bit, since the diffs get getting introduced in the manual swagger generation.

we're not going to depend on bits maintained by individual, which definitely introduces risk

Is it because it isn't published to a repository? There are several things in package.json that are under individuals accounts

@erich-wang
Copy link
Member

IMO, we should use the tool introduced by well known organization/company/person. We don't want to introduce any unnecessary risk into the CI.

@erich-wang what is an acceptable tool for you then? The CI is the important bit, since the diffs get getting introduced in the manual swagger generation.

we're not going to depend on bits maintained by individual, which definitely introduces risk

Is it because it isn't published to a repository? There are several things in package.json that are under individuals accounts

@nschonni
Copy link
Contributor Author

The author is a well known enough that they are one of the early GH sponsored developers https://github.com/users/caarlos0/sponsorship as they created the main release automation tool for the GoLang ecosystem

@erich-wang
Copy link
Member

@nschonni, if the person somehow delete the release (by accidentally), how should we handle it?

@nschonni
Copy link
Contributor Author

@eric-wang, that's a good question. 2 ways I can see to protect against that if you think it might happen:

  1. Store the .deb file in Azure DevOps artifacts instead of downloading from the GitHub release.
  2. Build your own docker container and push it to your teams Azure Container Registry

@erich-wang
Copy link
Member

@eric-wang, that's a good question. 2 ways I can see to protect against that if you think it might happen:

  1. Store the .deb file in Azure DevOps artifacts instead of downloading from the GitHub release.
  2. Build your own docker container and push it to your teams Azure Container Registry

Thank you, @nschonni. Could you help to create one issue to track the change request?

@erich-wang erich-wang mentioned this pull request Aug 6, 2019
Closed
@erich-wang
Copy link
Member

@nschonni , I just created new issue Add Json format check to CI to track the request, and I'll close this PR tomorrow, please let me know if any concern.

@erich-wang erich-wang closed this Aug 7, 2019
leni-msft pushed a commit to leni-msft/azure-rest-api-specs that referenced this pull request May 13, 2022
@rainbowFi
Fmc/new pmn api master (Azure#6496)
7f17f65 
* Copy 2022-01-01-preview API to new version folder

* Update API version number

* Remove deprecated TacList field

* Rename interfaces to be 4G/5G agnostic

* Remove SIM activation/deactivation API paths as this action is no longer supported

* Update configurationState field to be more generic and useful

* Fix linting errors

* Fix up interface documentation strings

* Commit changes from prettier

* Replace ConfigurationState with SimState in 2021-04-01-preview API

* Fix up accidental change

* Markups from ARM review

Co-authored-by: Fiona Corden <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erich-wang erich-wang Awaiting requested review from erich-wang

@DavidLiCIG DavidLiCIG Awaiting requested review from DavidLiCIG

@huangpf huangpf Awaiting requested review from huangpf

@hyonholee hyonholee Awaiting requested review from hyonholee

@mboersma mboersma Awaiting requested review from mboersma

@stankovski stankovski Awaiting requested review from stankovski

@yangyuan yangyuan Awaiting requested review from yangyuan

@jaredmoo jaredmoo Awaiting requested review from jaredmoo

Assignees

@erich-wang erich-wang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nschonni @AutorestCI @azuresdkci @erich-wang @jhendrixMSFT