Skip to content

Freddydk/avoidallsecrets #1735

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

Closed
wants to merge 38 commits into from
Closed

Freddydk/avoidallsecrets #1735

wants to merge 38 commits into from

Conversation

freddydk
Copy link
Contributor

@freddydk freddydk commented May 15, 2025

Avoid transfering all secrets available to the repository to ReadSecrets.

Instead Determine the secrets needed and read/transfer them.

Also refactor ReadSecrets to only attempt to read the missing secrets from Azure KeyVault as secrets from GitHub has already been resolved.

TODO

  • Refactor ReadSecrets to only get missing secrets from Azure DevOps
  • Check for max. number of secrets (32)

@Copilot Copilot AI review requested due to automatic review settings May 15, 2025 04:43
@freddydk freddydk requested a review from a team as a code owner May 15, 2025 04:43
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors how secrets are retrieved by introducing a step to determine exactly which secrets are needed and only reading missing ones from Azure KeyVault. It also updates existing workflows to wire the new DetermineSecrets action into ReadSecrets, removes redundant asterisk-based encryption markers in secrets lists, and refactors ReadSecrets and DetermineSecrets scripts for the new flow.

  • Add DetermineSecrets action calls before ReadSecrets in all workflows
  • Change ReadSecrets invocation to use a dynamic format string and indexed secret outputs (S0–S31)
  • Remove * encryption markers from secrets: lists and update PowerShell scripts to fetch missing secrets from KeyVault

Reviewed Changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
Templates/AppSource App/.github/workflows/PublishToAppSource.yaml Inserted DetermineSecrets step and updated ReadSecrets binding to use format(…,toJSON(secrets[env.S0]),…)
Templates/AppSource App/.github/workflows/NextMinor.yaml Removed * prefixes in secrets: list and added DetermineSecrets invocation
Actions/RunPipeline/RunPipeline.ps1 Fixed SecureString conversion flags and removed asterisk markers from pipeline secrets list
Actions/DetermineSecrets/DetermineSecrets.ps1 New script to collect requested secrets, map them, and emit FORMATSTR/S0–S31 outputs
Actions/ReadSecrets/ReadSecrets.ps1 Refactored to loop over indexed secrets, fetch missing ones from KeyVault, and mask them
Actions/ReadSecrets/ReadSecretsHelper.psm1 Simplified helper by removing old GetSecret/GetGithubSecret logic and adjusting KeyVault credential parsing
Comments suppressed due to low confidence (2)

Templates/AppSource App/.github/workflows/PublishToAppSource.yaml:79

  • The DetermineSecrets action declares its output as FORMATSTR but this workflow references formatStr, which may not match and could result in empty or broken secrets input. Ensure output casing and key name match exactly.
gitHubSecrets: ${{ format(steps.DetermineSecrets.outputs.formatStr,toJSON(secrets[env.S0]),… ) }}

Templates/AppSource App/.github/workflows/NextMinor.yaml:109

  • The asterisks (*) marking codeSignCertificatePassword and keyVaultCertificatePassword as encrypted were removed, which may disable encryption for these sensitive values. Confirm whether this change is intentional.
secrets: 'licenseFileUrl,codeSignCertificateUrl,codeSignCertificatePassword,...'

Copy link
Collaborator

@mazhelez mazhelez left a comment

Choose a reason for hiding this comment

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

Don't forget release notes.

@freddydk freddydk requested a review from mazhelez May 21, 2025 08:42
Co-authored-by: freddydk <[email protected]>
@freddydk freddydk requested a review from mazhelez May 21, 2025 11:47
@freddydk freddydk closed this May 22, 2025
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.

2 participants