Skip to content

Feature/ci mgmt#5378

Merged
weidongxu-microsoft merged 32 commits intomasterfrom
feature/ci-mgmt
Nov 14, 2019
Merged

Feature/ci mgmt#5378
weidongxu-microsoft merged 32 commits intomasterfrom
feature/ci-mgmt

Conversation

@weidongxu-microsoft
Copy link
Member

Create feature branch to test out ci.mgmt pipeline.

@weidongxu-microsoft
Copy link
Member Author

@mitchdenny
As your suggestion I've made a feature branch to test out the CI integration. Still trying to figure the rules, since ci.data and ci.client should not run here, and new ci.mgmt should run.

@weshaggard
Copy link
Member

@weidongxu-microsoft do you have any ETA on this work? We really would like to get the root of the repo cleared up and into this new structure so folks coming to the repo can more easily see the readme file and the structure will become more consistent with the other language repos. Java is the last repo to conform to this new structure.

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Oct 10, 2019

@weshaggard I am still working on it (but at a lower priority). Code restructuring is solved. But there is lots of try-and-error while configuring the pipeline (with too much yaml intangled and too little documationation for guidance), with the gap need to be tried-out and I am not comfortable to do this even on the feature branch. i probably will try that on another fork under my github account to faciliate (without affecting others) the configuation on which file change triggers which pipeline among all these pipelines.

@weshaggard
Copy link
Member

I'd suggest we do this in steps if we can. First step is to restructure then the next step is change the pipelines.

@weidongxu-microsoft
Copy link
Member Author

@mitchdenny @joshfree

What is your opinion?

Do we try to do it in steps (with first step just move the repositories and config the mgmt.yaml with say 50 lines of explicit excludes on client/data-plane lib), then config the pipeline for the moved mgmt lib; or we still try to figure out the pipeline in this PR as well?

If later does anyone have better way to test the triggering of pipeline without actually sending dummy PR to do the trigger? It is because this PR also trigger client/data-plane build which was excluded and should not be trigger and I've no idea why.

@mitchdenny
Copy link
Contributor

Because you are developing on a branch, you can just queue your build against that branch specifically. You do this via the Azure Pipelines UX. Find your pipeline, run it, and select the branch.

@mitchdenny
Copy link
Contributor

/check-enforcer evaluate

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Oct 24, 2019

@mitchdenny

I've manually added the ADO pipeline for keyvault mgmt.
https://dev.azure.com/azure-sdk/internal/_build?definitionId=1217&_a=summary
https://dev.azure.com/azure-sdk/public/_build?definitionId=1218&_a=summary
Please take a review. Thanks.

pom change on sdk/keyvault/pom.service.xml

There appears to be 2 pipeline with same config, one in public, one in internal. What is the purpose?

I still do not know how to use PipelineGenerator. Would you give me a sample command line with parameters (e.g. I've no idea what "Name of an environment variable which contains a PAT." is)?
It will be needed when we do refactor in batch.

@weidongxu-microsoft
Copy link
Member Author

@JimSuplizio @weshaggard

Here is still restructure + pipeline combined. Please let me know your opinion on it.

Note since the transfer to azure.core is not done on mgmt yet, these are all legacy mgmt SDKs (and as Jim mentioned in email, some of them will not build. One cause might be that the "azure-arm-client-runtime" package later than 1.6.5 no longer have "test" jar published, hence for pom had to specify 1.6.5 as test.)

And CI is not using the shared yaml template, but using a similar configure as eng/pipelines/mgmt.yml
I am not sure whether in future we would have certain period that the legacy code / pipeline would co-exist with new code / pipeline based on azure.core, and whether we need to prepare for it at present (say, ci.mgmt-legacy.yml, instead?).

@mitchdenny
Copy link
Contributor

@weidongxu-microsoft I've had a look through this and I think that this is a reasonable approach for now. Once this is merged, I'd actually like to evaluate whether we can convert the management, track 1 and track 2 client pipelines on a per service directory basis.

The reason is that I am conscious of a current performance issue in Azure Pipelines where they make a request to GitHub for each pipeline associated with the repository (and templates it references). Until they optimize this doubling up on all the pipelines could be a perf/stability issue. I think in time that will be fixed though.

We shouldn't block on this now - I just wanted to give you an indication of where my head is at.

@weidongxu-microsoft
Copy link
Member Author

@mitchdenny Thanks for your comments. I will wait a few days for @JimSuplizio @weshaggard 's input as well.

BTW, do you have sample usage on PipelineGenerator? I assume this is the tool to create CI pipeline automatically in batch. And when we resturcture multiple services, We will need the tool to configure the CI pipeline (instead of manually here).

@mitchdenny
Copy link
Contributor

I'll send you a link offline, our invocation of the pipeline generator sits in a template on a private repo.

@weshaggard
Copy link
Member

I agree with the approach of just updating the existing mgmt.yml files for now to account for the restructuring. It looks like you have currently only moved the keyvault directories so I assume you plan to do the others as well? Perhaps in another PR.

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Nov 1, 2019

@weshaggard

This PR is actually separating the pipeline (and make new dedicated CI for keyvault mgmt) as well.

If this one is OK, next plan is PR no.2 to move one more service, and configure the pipeline generator job to pick up it (with yaml) and create an CI in DevOp automatically.
And then we could do in batch in PR no.3 ++.

History is this:
At first we planned to move code only, and continue using mgmt.yml for CI. However this got complicated by DevOp not supporting wildcard in yaml include/exclude clause so we had difficulty to either include the specific mgmt folder or exclude data/client folder under sdk.
And after that we are trying to sperate the CI as well. And complications with that make this PR take quite some time to form.
Now I think Mitch and I could agree that the sperated mgmt CI in keyvault is in good shape and it is now possible to continue with other services.

@JimSuplizio I would still like your opinion. I do not want to surprise you with any CI change or change in sdk folder, since you probably work on CI and other utils under sdk folder as well (e.g. I do not want to move this and suddently your python script now updating thing that it does not suppose to update).

@weidongxu-microsoft
Copy link
Member Author

Jim raised an issue that there be quite many CI pipeline does not define SdkType=client. I will take some time to review this case as well.

@mitchdenny
Copy link
Contributor

Jim raised an issue that there be quite many CI pipeline does not define SdkType=client. I will take some time to review this case as well.

This is by design. The default (from memory) is that the pipelines are treated as track 1 pipelines (client). But for those that aren't we define them as data. All it really controls is the spin up of Jetty at this point since the new track 2 libraries don't require it.

@weidongxu-microsoft
Copy link
Member Author

weidongxu-microsoft commented Nov 6, 2019

@mitchdenny Thanks. The issue is that previously there is at most 2-value in profile selecting, so it is such:

    <profile>
      <id>data</id>
      <activation>
        <property>
          <name>env.SDKTYPE</name>
          <value>data</value>
        </property>
      </activation>
      ...
    </profile>
    <profile>
      <id>client</id>
      <activation>
        <property>
          <name>env.SDKTYPE</name>
          <value>!data</value>
        </property>
      </activation>
      ...
    </profile>

And the activation for client is loose as !data. So SdkType could be client, could be not defined, could be anything other than data.

Now after we added mgmt, it would be 3-value, and maven does not support e.g. !data && !mgmt in profile activation. So I modified it to such:

    <profile>
      <id>data</id>
      <activation>
        <property>
          <name>env.SDKTYPE</name>
          <value>data</value>
        </property>
      </activation>
      ...
    </profile>
    <profile>
      <id>client</id>
      <activation>
        <property>
          <name>env.SDKTYPE</name>
          <value>client</value>
        </property>
      </activation>
      ...
    </profile>
    <profile>
      <id>mgmt</id>
      <activation>
        <property>
          <name>env.SDKTYPE</name>
          <value>mgmt</value>
        </property>
      </activation>
      ...
    </profile>

And the activation for client would thus be strictly as client.

So far I think this would not cause problem, since either CI for client already defined SdkType=client, or we can spec !mgmt to existing CI without SdkType defined and add mgmt for newly added ci.mgmt.

Copy link
Member

Choose a reason for hiding this comment

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

nit: can you align the spacing with the rest of the file? It looks like you're off by a tab or two

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. It's actually a few tabs on line above. I will change to spaces for all.

@joshfree
Copy link
Member

Thank you @weidongxu-microsoft for starting the migration of management plane SDKs under the /sdk root folder! This will really help clean up the azure-sdk-for-java repo and get it clean and consistent with the other azure-sdk-for-* repo directory structures. 😄

@weidongxu-microsoft weidongxu-microsoft merged commit 17438f2 into master Nov 14, 2019
@weidongxu-microsoft weidongxu-microsoft deleted the feature/ci-mgmt branch November 14, 2019 02:08
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.

6 participants

Comments