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

Allow Multiple Invocations of archetype-sdk-tests-generate within the same stage #1598

Merged
merged 1 commit into from
May 14, 2021

Conversation

scbedd
Copy link
Member

@scbedd scbedd commented May 8, 2021

This is a super useful template, but I need to run it twice!

See example PR over here

And example run of what I want in an example run here.

Without these changes, we run into "duplicate job id" when we attempt to run it in the same stage. Discussion for the why I want to run it twice in the same stage is also present on the linked PR.

@scbedd scbedd requested a review from a team as a code owner May 8, 2021 23:51
@scbedd scbedd changed the title Allow Multiple Invocations of archetype-sdk-tests-generate Allow Multiple Invocations of archetype-sdk-tests-generate within the same stage May 8, 2021
@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

@mitchdenny
Copy link
Contributor

LGTM

@weshaggard weshaggard requested a review from benbp May 10, 2021 16:20
@benbp
Copy link
Member

benbp commented May 10, 2021

I have run into this problem before too, thanks for solving it!

I wonder if we can avoid the requirement of another added parameter GenerateJobName, since it could be confusing if you don't know what it does (and vice versa if you don't know you have to add it to support multiple generate jobs).

A couple ways I can think that would do this:

  1. Use a counter variable in the job name. This is the simplest as far as config goes, but the job names generate_matrix_1, generate_matrix_2 aren't very descriptive if you're trying to debug failues/raw matrix output.
  2. We could generate the name based on either the JobTemplatePath or a Path or Name from the MatrixConfigs. This would have the drawback that it won't work if you need to use the same template or matrix file multiple times.

I'm fine with this as is but if we can find a decent way to avoid the parameter that would be ideal. Please add a comment above the GenerateJobName parameter explaining its purpose if you keep that approach.

@scbedd
Copy link
Member Author

scbedd commented May 11, 2021

@benbp I think the most reasonable here is probably just using the counter. I feel like we'd be backing ourselves into a corner where we inevitably want to use the same matrix, but can't due to that restriction.

I'm going to try to use a counter variable in another job. If that works, I'll add that change. If it doesn't, I'll just merge this as-is (along with a comment :P)

@benbp
Copy link
Member

benbp commented May 11, 2021

@scbedd if you have issues with the counter let me know and I can possibly help debug. Seems like you have to declare it in variables in order for it to work based on the docs.

@scbedd
Copy link
Member Author

scbedd commented May 11, 2021

@benbp right on. That's honestly my biggest concern. if I can put a variables just within that specific test, we're fine. Otherwise it won't minimize confusion at all, and I'd rather keep the parameter :P

@scbedd
Copy link
Member Author

scbedd commented May 13, 2021

@benbp Evidence of newest update successfully building here.

Ok. That original test wasn't running on the branch I thought it was.

Doesn't look like this is possible with a runtime variable.

This commit attempts to access the counter variable by accessing it in "run-time" mode. This is necessary as these counters are by their very definition run-time defined.

  - ${{ if eq(config.GenerateVMJobs, 'true') }}:
    - template: ${{ parameters.JobTemplatePath }}
      parameters:
        UsePlatformContainer: false
        Matrix: dependencies.$[variables.invocationCounter].outputs['generate_vm_job_matrix_${{ config.Name }}.matrix']
        DependsOn: generate_matrix_$[variables.invocationCounter]
        CloudConfig: ${{ parameters.CloudConfig }}
        ${{ each param in parameters.AdditionalParameters }}:
          ${{ param.key }}: ${{ param.value }}

The issue is with Matrix: dependencies.$[variables.invocationCounter].outputs['generate_vm_job_matrix_${{ config.Name }}.matrix']

The config.Name resolution works because it's available when the template is compiled. Unfortunately for us, all methods of accessing variable (I tried $(varName) and $[variables.varName]) don't work.

Error is

/eng/pipelines/templates/jobs/ci.tests.yml (Line: 50, Col: 15): Unexpected symbol: '$'. Located at position 14 within expression: dependencies.$[variables.invocationCounter].outputs['generate_vm_job_matrix_Python_ci_test_base.matrix']. For more help, refer to https://go.microsoft.com/fwlink/?linkid=842996

Going to revert to original method and add a comment.

@scbedd scbedd force-pushed the feature/extend-matrix-configuration branch from ab7d914 to b555cd8 Compare May 13, 2021 23:07
@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

@ghost
Copy link

ghost commented May 14, 2021

Hello @azure-sdk!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@scbedd
Copy link
Member Author

scbedd commented May 14, 2021

/check-enforcer override

@scbedd scbedd merged commit 6fc24f3 into Azure:master May 14, 2021
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.

5 participants