microsoft / navcontainerhelper

Official Microsoft repository for BcContainerHelper, a PowerShell module, which makes it easier to work with Business Central Containers on Docker.
MIT License
381 stars 243 forks source link

Default behavior change of (at least) both Compile-App* commands #3667

Open MODUSCarstenScholling opened 5 days ago

MODUSCarstenScholling commented 5 days ago

Describe the issue Based on PR #3430, the default behavior of at least both Compile-App* commands has changed on Azure DevOps. See commit.

In a way that cannot be seen when debugging because simple debugging has not changed. By automatically assigning environment vars for Azure DevOps to $BCContainerHelperConfig.IsAzureDevOps, the default output changes in the pipeline context only.

This change is not even mentioned in the release notes. I would like to see this prominently in the notes or even the default assignment reversed (as you @freddydk mentioned IsGitHubActions and IsAzureDevOps - both default to $false.).

Thank you.

freddydk commented 5 days ago

Yeah, this should have been mentioned in the release notes that there are three new settings, but could you explain to me what the problem is? I mean, how can this break the behavior? The parameters on Compile where false before and now, they are defaulting to values from config, which are false by default

MODUSCarstenScholling commented 5 days ago

Of course. In pipeline runs, we are processing the raw compiler output, creating an object and finally an Excel sheet. And due to this change, the behavior in a pipeline changes, the format for errors are warnings does not match our regex anymore and only Infos are processed.

When debugging this outside of the pipeline, it works perfectly because env:TF_BUILD is not set. On top of it, you cannot see any difference in the pipeline output.

The defaults are according to the environment, which is not $false, but this, $true on DevOps run, false in plain PS:

            "IsGitHubActions" = ($env:GITHUB_ACTIONS -eq "true")
            "IsAzureDevOps" = ($env:TF_BUILD -eq "true")
            "IsGitLab" = ($env:GITLAB_CI -eq "true")
freddydk commented 5 days ago

So, you are saying that in the old days - pipelines where running on AzureDevOps worked for you because the AzureDevOps flag was NOT set. And now - because the AzureDevOps flag is set, it doesn't work anymore? and if you set $bcContainerHelperConfig.IsAzureDevOps to $false (or add it to the config file) then everything works again?

MODUSCarstenScholling commented 5 days ago

If you call last week "the old days" then yes. Before that change, the switches were not initialized, both AzureDevOps and gitHubActions were unset and your code returned the raw output:

    if ($AzureDevOps -or $gitHubActions) {
        $devOpsResult | ForEach-Object { $outputTo.Invoke($_) }
    }
    else {
        $result | ForEach-Object { $outputTo.Invoke($_) }
        if ($devOpsResult -like "*task.complete result=Failed*") {
            throw "App generation failed"
        }
    }

Now the config IsAzureDevOps is preinitialized with $env:TF_BUILD which is $true when running in the pipeline (changeing the behavior) and $false outside.

I am not asking to revert it, but to have the same default values again and/or very clear a mention in the release notes. It took me and my company 10 hours to get a clear picture on what happend and I want to avoid that for others.

freddydk commented 5 days ago

I will update releasenotes to include this change.

I do think it is a good thing to have AzureDevOps set to true when running Azure DevOps, GitHub Actions set to true when running GitHub etc. - but I can see that this apparently had some side effects in when things would fail, I wasn't aware of that.

Earlier it would output the raw output and throw an exception Now it just outputs the Azure DevOps error text and doesn't throw an exception

Is this the essence of the problem?

freddydk commented 5 days ago

So you basically want to handle the raw output yourself and not rely on the conversion in ContainerHelper.

MODUSCarstenScholling commented 5 days ago

Yes, I do. But now specifying both switches as -switch:$false. Additionally to the raw output, we also dump the Azure DevOps output with the Convert-ALC* command.

lktraser commented 2 days ago

Not to highjack this topic but there is one more problem: I can suppress the warnings in compile with Compile-AppInBcContainer and Compile-AppWithBcCompilerFolder via the parameter but Validate-ALApp uses the compile but doesn't have the parameter so the warnings are always active now on DevOps if validation is run.

I can create a PR for this if you like.

freddydk commented 1 day ago

@lktraser yeah, please do - I haven't heard the request from anybody, but would be fine.