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

azartifacts preview v0.1.0 #14677

Merged
merged 2 commits into from
May 20, 2021
Merged

Conversation

jhendrixMSFT
Copy link
Member

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to CHANGELOG.md are included.
  • MIT license headers are included in each file.

@ghost ghost added the Synapse label May 18, 2021
@check-enforcer
Copy link

This repository is protected by Check Enforcer. The check-enforcer check-run will not pass until there is at least one more check-run successfully passing. Check Enforcer supports the following comment commands:

  • /check-enforcer evaluate; tells Check Enforcer to evaluate this pull request.
  • /check-enforcer override; by-pass Check Enforcer (approvals still required).

@jhendrixMSFT
Copy link
Member Author

/check-enforcer override

@ArcturusZhang
Copy link
Member

ArcturusZhang commented May 19, 2021

Hi @jhendrixMSFT do we need to add some synapse data-plane specific information in this PR? For instance like the things added to go-autorest previously by this PR: Azure/go-autorest#552

And also, the CI pipelines are not working for synapse in this PR

@jhendrixMSFT
Copy link
Member Author

CI has not yet been hooked up for this module as the yaml hasn't been committed yet.

The codegen is self-contained, so no need to add anything else. Long-term, I don't know that we'll add a similar environment table like we have in go-autorest.

@catalinaperalta
Copy link
Member

catalinaperalta commented May 19, 2021

I think this needs a README with at least the same information the ARM SDKs have....to include information like how to use the ctor for a new connection, where azidentity is, etc. I think for now it could be a copy of one of the ARM ones.

I say this in part, because I was looking at the connection file and then wondered if people would easily know that azidentity implements azcore.Credential and they wont go off making their own credential or using something else. Aside from the fact that even the simpler READMEs we have for the ARM libs provide some basic and useful info.

@jhendrixMSFT
Copy link
Member Author

The README has been added.

Copy link
Member

@catalinaperalta catalinaperalta left a comment

Choose a reason for hiding this comment

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

LGTM

@jhendrixMSFT jhendrixMSFT merged commit 4291d7a into Azure:master May 20, 2021
@jhendrixMSFT jhendrixMSFT deleted the synapse_azartifacts branch May 20, 2021 02:08

## Prerequisites

- an [Azure subscription](https://azure.microsoft.com/free/)
Copy link
Member

Choose a reason for hiding this comment

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

We might need to capitalize the first letter here

Suggested change
- an [Azure subscription](https://azure.microsoft.com/free/)
- An [Azure subscription](https://azure.microsoft.com/free/)


The `azartifacts` module provides operations for working with Azure synapse.

[Source code](https://github.com/Azure/azure-sdk-for-go/tree/master/sdk/synapse/azartifacts)
Copy link
Member

Choose a reason for hiding this comment

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

The CI is not working therefore this link is not reported as invalid. Therefore the CI pipelines are designed not to work in the first PR of a module?

Copy link
Member Author

Choose a reason for hiding this comment

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

We still need to figure out how this will work long-term. At present, we can assume that CI won't be hooked up for the first module release :(

Copy link
Member

Choose a reason for hiding this comment

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

That would be the most unfortunate... do we have to locally run go vet to verify the SDK could compile properly?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I did yes. Long term, we'll want to have an easy way to set up CI before releasing a module. I didn't bother with it here given our requirement for expedience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants