-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Provide errors for missing and unexpected parameters in azure pipelines #238
Provide errors for missing and unexpected parameters in azure pipelines #238
Conversation
if (svalue.Contains("${{")) | ||
{ | ||
eventInfo = new YamlDotNet.Serialization.ScalarEventInfo(new ReplaceDescriptor { Value = svalue.Replace("${{", "${{ '${{' }}"), Type = eventInfo.Source.Type, StaticType = eventInfo.Source.StaticType, ScalarStyle = eventInfo.Source.ScalarStyle }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I borrowed this from the Blazor app you provided, but I haven't seen the scenario where this is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have something like that
steps:
- script: echo '${{ '${{ something }}' }}' # We Escape the expression literal, comes from GitHub Actions forums
Then it is required to reescape, so the output keeps valid yaml for the template engine to feed the result back and process the result again.
I think the official preview api, doesn't consider this scenario at all.
@@ -0,0 +1,61 @@ | |||
namespace Runner.Server.Azure.Devops | |||
{ | |||
// TODO: Move to Sdk.AzurePipelines folder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this into the SDK project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving it to the sdk is ok for me.
I think this Pipeline pipeline
should be done via a new interface type with method ToContextData()
So any pipeline component can be converted to yaml, not just pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IContextDataProvider
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, can be part of a later change
@@ -540,7 +540,6 @@ public class AzureDevops { | |||
throw new Exception("Provided undeclared parameters"); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you provide an example where parameters is referred to as a mapping and not a sequence? I didn't touch this because I couldn't reproduce this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameters:
val:
- a
- b
steps:
- script: echo ${{ converttojson(parameters.val) }}
old syntax, I can run this online via azure pipelines. Only Provided undeclared parameters
is never reported.
src/Test/Test.csproj
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You broke the old Test project (see ci failure), could be due to breaking xunit changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are expected to fail if you never ran the ./dev test
script, it download external tools like nodejs.
To be honest some of the original actions/runner tests fail for me locally due to my non English system language on Windows and that are more failures than you see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it originally because I'm using Visual Studio 2022 and none of the tests were loading. I'm not seeing that issue anymore
I fixed the Tests, by updating one specfic dependency of the test host and removing a lot of broken tests of partial merges / non relevant tests for trimmed package updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChristopherHX fyi - i wasn't aware of the ./dev[.cmd]
script.
I managed to get all the tests to pass. I ran the dotnet msbuild src/dir.proj -t:GenerateConstant
which resolved one of the tests with BuildConstants.cs
and running ./dev.cmd build
and ./dev.cmd layout
resolved the others.
@@ -0,0 +1,8 @@ | |||
# ExpectedException: NotSupportedException | |||
# ExpectedErrorMessage: __built-in-schema.yml (Line: 40, Col:11): 'variables' is already defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is not really expected, it needs to change.
(Line: 40, Col:11) is just wrong, __built-in-schema.yml
is an implementation details of the ms engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is expected. I introduced this exception in #227 to mimic the exception produced by Azure Pipelines when the calling pipeline and the pipeline being extended both declare variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I aggree an error is expected.
I disagree about the hardcoded file location
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. If there is a specific error then there is an opportunity to represent that feedback somewhere. For example, a vscode extension could highlight the offending line. I'll take a closer look.
Please add a calling entry in runner.server/.github/workflows/build.yml Lines 84 to 88 in c7ee3e7
so the GitHub Actions are actually running your tests. Should be run by some dotnet sdk command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the unit test framework, I have changed it to work on linux and macOS (by not hardcoding the \
in path management)
This PR includes fixes and supporting tests for:
This also includes a dedicated xUnit test fixture for the SDK and a basic framework for validating azure pipeline YAML parsing. A test-fixture that automatically detects azure pipelines in
testworkflows/azpipelines
is included:These regression tests are different than the current tests in the PR build validation in that they include assertions for error conditions.
Each regression test can optionally provide meta-data in the form of comments at the top of the test:
Name: Display name for the test (Optional)
ExpectedException. Class name for the exception if this scenario is designed to throw an exception. (Optional)
GitHub.DistributedTask.ObjectTemplating
orSystem
namespace.ExpectedErrorMessage: The error message for the expected exception. Only valid if the
ExpectedException
is specifed. (Optional)LocalRepository: Mapping between repositoryAndRef and folder location relative to
testworkflows/azpipelines
(Optional).project/name@ref=folder
where folder is relative totestworkflows/azpipelines