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

Remove thisbuild artifacts and support preprocessor symbols #1374

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

freddydk
Copy link
Contributor

@freddydk freddydk commented Dec 29, 2024

This is a proposed fix for https://github.com/orgs/microsoft/projects/521/views/1?pane=issue&itemId=91449371 and will greatly improve the capabilities of build modes (as requested by partners during our roundtable at Directions EMEA).
 

New Repository Settings

  • shortLivedArtifactsRetentionDays determines the number of days to keep short lived build artifacts (f.ex build artifacts from pull request builds, next minor or next major builds). 1 is default. 0 means use GitHub default.
  • longLivedArtifactsRetentionDays determines the number of days to keep long lived build artifacts (f.ex build artifacts from CI/CD builds). 0 is the default and means use GitHub default.
  • preProcessorSymbols is a list of preprocessor symbols to use when building the apps. This setting can be specified in workflow specific settings files or in conditional settings.
  • <buildMode>PreProcessorSymbols is a list of preprocessor symbols to be used when building apps in the <buildMode> build mode. CleanModePreProcessorSymbols is a variation of this.

Change in published artifacts

When using useProjectDependencies in a multi-project repository, AL-Go for GitHub used to generate short lived build artifacts called thisBuild-<projectnaame>-<type>-.... This is no longer the case. Instead, normal build artifacts will be published and used by depending projects. The retention period for the artifacts generated are controlled by two settings called shortLivedArtifactsRetentionDays and longLivedArtifactsRetentionDays.

Preprocessor symbols

It is now possible to define preprocessor symbols, which will be used when building your apps using the preProcessorSymbols setting. This setting can be specified in workflow specific settings file or it can be used in conditional settings.

You can also specify preProcessor symbols for custom build modes by using the <buildMode>PreProcessorSymbols setting.

TODO

  • Get partner confirmation
  • Fix existing tests
  • Add to end 2 end

@Copilot Copilot bot review requested due to automatic review settings December 29, 2024 22:11
@freddydk freddydk requested a review from a team as a code owner December 29, 2024 22:11
Copy link

@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.

Copilot reviewed 8 out of 23 changed files in this pull request and generated 1 comment.

Files not reviewed (15)
  • Actions/AL-Go-Helper.ps1: Language not supported
  • Actions/CalculateArtifactNames/CalculateArtifactNames.ps1: Language not supported
  • Actions/Github-Helper.psm1: Language not supported
  • Actions/ReadSettings/ReadSettings.ps1: Language not supported
  • Actions/RunPipeline/RunPipeline.ps1: Language not supported
  • Templates/AppSource App/.github/workflows/Current.yaml: Language not supported
  • Templates/AppSource App/.github/workflows/NextMajor.yaml: Language not supported
  • Templates/AppSource App/.github/workflows/NextMinor.yaml: Language not supported
  • Actions/CalculateArtifactNames/README.md: Evaluated as low risk
  • Actions/CalculateArtifactNames/action.yaml: Evaluated as low risk
  • Actions/ReadSettings/README.md: Evaluated as low risk
  • Templates/Per Tenant Extension/.github/workflows/NextMinor.yaml: Evaluated as low risk
  • Scenarios/settings.md: Evaluated as low risk
  • Actions/ReadSettings/action.yaml: Evaluated as low risk
  • Templates/AppSource App/.github/workflows/_BuildALGoProject.yaml: Evaluated as low risk
Comments suppressed due to low confidence (1)

Templates/Per Tenant Extension/.github/workflows/NextMajor.yaml:103

  • The usage of fromJson for artifactsRetentionDays should be verified to ensure it correctly parses the value.
artifactsRetentionDays: ${{ fromJson(needs.Initialization.outputs.artifactsRetentionDays) }}

Actions/Github-Helper.psm1 Fixed Show fixed Hide fixed
@freddydk freddydk marked this pull request as draft December 30, 2024 12:49
@freddydk freddydk changed the title Remove thisbuild artifacts Remove thisbuild artifacts and support preprocessor symbols Dec 30, 2024
- [`shortLivedArtifactsRetentionDays`](https://aka.ms/algosettings#shortLivedArtifactsRetentionDays) determines the number of days to keep short lived build artifacts (f.ex build artifacts from pull request builds, next minor or next major builds). 1 is default. 0 means use GitHub default.
- [`longLivedArtifactsRetentionDays`](https://aka.ms/algosettings#longLivedArtifactsRetentionDays) determines the number of days to keep long lived build artifacts (f.ex build artifacts from CI/CD builds). 0 is the default and means use GitHub default.
- [`preProcessorSymbols`](https://aka.ms/algosettings#preProcessorSymbols) is a list of preprocessor symbols to use when building the apps. This setting can be specified in workflow specific settings files or in conditional settings.
- [`<buildMode>PreProcessorSymbols`](https://aka.ms/algosettings#cleanModePreProcessorSymbols) is a list of preprocessor symbols to be used when building apps in the \<buildMode> build mode. CleanModePreProcessorSymbols is a variation of this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that this PR also adds buildmode as a condition for conditional settings, shouldn't people just create a conditional setting called "preProcessorSymbols"? Similar to what you did here https://github.com/BusinessCentralApps/buildorder/blob/8e64503f243aaf6e24e7deca8d47b80975c97c54/BO-DK/.AL-Go/settings.json#L8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could - but then we would have cleanModePreprocessorSymbols as a special case for compatibility?
Should we then issue a deprecation warning about that and ask people to modify this to a conditionalsetting instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we then issue a deprecation warning about that and ask people to modify this to a conditionalsetting instead?

That sounds like a good plan I think

### New Repository Settings

- [`shortLivedArtifactsRetentionDays`](https://aka.ms/algosettings#shortLivedArtifactsRetentionDays) determines the number of days to keep short lived build artifacts (f.ex build artifacts from pull request builds, next minor or next major builds). 1 is default. 0 means use GitHub default.
- [`longLivedArtifactsRetentionDays`](https://aka.ms/algosettings#longLivedArtifactsRetentionDays) determines the number of days to keep long lived build artifacts (f.ex build artifacts from CI/CD builds). 0 is the default and means use GitHub default.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we use GitHubs default, why not just let people control this setting through GitHub? Is there a need for an AL-Go setting?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we could remove the longLivedArtifactsRetentionDays

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.

Stop producing thisbuild artifacts and instead always produce artifacts with configurable retention
2 participants