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

Use isOutput=true when writing variables in Azure DevOps #859

Closed
goldsam opened this issue Nov 9, 2022 · 6 comments · Fixed by #888
Closed

Use isOutput=true when writing variables in Azure DevOps #859

goldsam opened this issue Nov 9, 2022 · 6 comments · Fixed by #888

Comments

@goldsam
Copy link

goldsam commented Nov 9, 2022

Using variables written in one job/stage in Azure Devops from a later job/stage currently requires re-exporting NBGV_* variables resulting in needlessly complex azure pipeline files. Would it be possible to add a cli flag to that variables written in a devops environment will include isoutput=true?

public IReadOnlyDictionary<string, string> SetCloudBuildVariable(string name, string value, TextWriter stdout, TextWriter stderr)
{
Utilities.FileOperationWithRetry(() =>
(stdout ?? Console.Out).WriteLine($"##vso[task.setvariable variable={name};]{value}"));
return GetDictionaryFor(name, value);
}

@AArnott
Copy link
Collaborator

AArnott commented Nov 9, 2022

That's a great idea. I don't think we need a flag for this, since converting a variable to an output variables only expands scope AFAIK. Would you care to send the PR?

@goldsam
Copy link
Author

goldsam commented Nov 24, 2022

Unfortunately, output variable references must include the name of the step in which they were written. This is different from standard variable references which don't required any qualifier. The following pipeline snippet demonstrates this:

steps:
- bash: |
    echo '##vso[task.setvariable variable=myCondStandardVar]true'
    echo '##vso[task.setvariable variable=myCondOutputVar;isOutput=true]true'
  displayName: "settting variables"
  name: myStep
- bash: |
    echo myCondStandardVar: $(myCondStandardVar)
    echo myCondOutputVar: $(myStep.myCondOutputVar)

I believe simply adding isOutput=true would introduce a breaking change. Aside from introducing a flag, the only solution I can think of would be to write each variable twice as a standard and output variable (using distinct names). That idea seems unclean to me. What are your thoughts?

@AArnott
Copy link
Collaborator

AArnott commented Nov 24, 2022

I think writing it out twice is a fine idea. We certainly don't want to break backward compat, and offering an option that gives the output variable but at the expense of breaking whatever the user has that depends on regular variables is sad too. I just wonder why AzP doesn't allow for writing out a variable that does both jobs with one log command, but I don't mind if we do it twice.

@ronaldbarendse
Copy link
Contributor

ronaldbarendse commented Jan 3, 2023

What about checking an environment variable to switch to output variables instead, e.g. NBGV_CloudBuild_IsOutput? And maybe add the possibility to specify specific variables to set as output, so besides true/false, also allow setting NBGV_VersionMajor;NBGV_NuGetPackageVersion to prevent outputting all variables?

jobs:
- job: A
  variables:
    NBGV_CloudBuild_IsOutput: true
  steps:
  - script: dotnet tool install --tool-path . nbgv
  - script: ./nbgv get-version
    name: getversion
- job: B
  dependsOn: A
  variables:
    versionMajor: $[ dependencies.A.outputs['getversion.NBGV_VersionMajor'] ]  
  steps:
  - script: echo $(versionMajor)

Implementing it this way will only require changes to the build pipeline to opt-in to writing output variables and changing the syntax to use the task name...

@AArnott
Copy link
Collaborator

AArnott commented Jan 7, 2023

That requires more work on the user's part. What risk are we mitigating by doing that?

@goldsam
Copy link
Author

goldsam commented Jan 20, 2023

I've been experimenting. Simply using the same name and outputting twice works fine without risk of conflicts.

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

Successfully merging a pull request may close this issue.

3 participants