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

Azure pipelines migration #1267

Merged
merged 33 commits into from
Feb 2, 2020

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Jun 20, 2019

PR Summary

This is the minimum viable, lift and shift approach to have test running in Azure DevOps.
For better visibility in a PR I decided to create multiple build definitions instead of using multiple jobs in one build definition and another reason is that if one used multiple job in one build definition, one would not be able to see from which platform they came.
Good news: They pass on macOS but there are some test failures to be looked into for Ubuntu. I suggest to only validate against PS 5/6 on Windows and Mac and keep track of the Ubuntu failures until we have fixed them. Somehow there is also a bug in Azure DevOps where the task to upload test results it supposed to always run using succeededOrFailed but in practice it does not run if the previous task fails...

After this I suggest we only keep the PS v4 test in AppVeyor

@rjmholt Can I ask you please to look into the test failures from your compatibility module please? It seems that those tests pass on Ubuntu 16 but not Ubuntu 18, see related PR #1262 where I tried to fix the Ubuntu builds in AppVeyor. Also: I was not sure if we still need the calls to ./PSCompatibilityCollector/build.ps1 in the build yaml script?

PR Checklist

@bergmeister bergmeister requested a review from rjmholt June 22, 2019 07:12
@rjmholt
Copy link
Contributor

rjmholt commented Jun 28, 2019

My current suspicion is that the Ubuntu 18.04 problems are caused by the LANG=C.UTF-8 locale change on that platform. I'll have a look into how dictionaries are built to make sure they're not taking the culture into account in any way.

@rjmholt
Copy link
Contributor

rjmholt commented Jun 28, 2019

Hmmmm just confirmed it wasn't that and haven't been able to reproduce yet on my ubuntu 18.04 VM

@rjmholt
Copy link
Contributor

rjmholt commented Jun 28, 2019

Also: I was not sure if we still need the calls to ./PSCompatibilityCollector/build.ps1 in the build yaml script?

We should certainly be building the module to make sure it builds in CI, but not sure if you mean that there's another call to that build script elsewhere that outmodes it?

@bergmeister bergmeister reopened this Aug 22, 2019
# Conflicts:
#	Tests/Engine/CustomizedRule.tests.ps1
@bergmeister
Copy link
Collaborator Author

Close and re-open to see if things have changed and if the new Ubuntu failures on AppVeyor, happen here as well.

@bergmeister bergmeister reopened this Oct 27, 2019
@bergmeister bergmeister closed this Jan 2, 2020
@bergmeister bergmeister reopened this Jan 2, 2020
@rjmholt
Copy link
Contributor

rjmholt commented Jan 3, 2020

Hmmm, so it is reproducing in AzDO CI as well. After spending a day trying to reproduce this, I stopped trying when @JamesWTruher and I didn't see it in AzDO (only AppVeyor).

Since this seems to be causing an issue in AzDO now, I'll go back to trying to reproduce it. I didn't have much luck replicating the AzDO environment locally...

@bergmeister
Copy link
Collaborator Author

Maybe just ignoring the failures similar to the AppVeyor ones would be the best? Interestingly there were new AppVeyor AppVeyor failures just before xmas in AppVeyor but they have disappeared now

@rjmholt
Copy link
Contributor

rjmholt commented Jan 3, 2020

Frustrating. But yes, I don't think these failures should block anything, so we should ignore them for now.

@rjmholt
Copy link
Contributor

rjmholt commented Jan 3, 2020

I'll open a PR to ignore them in AzDO

@bergmeister
Copy link
Collaborator Author

I suggest to just add them in this PR here and use an environment variable that is only set in Az-Do

@rjmholt
Copy link
Contributor

rjmholt commented Jan 3, 2020

I suggest to just add them in this PR here and use an environment variable that is only set in Az-Do

Ok with you if I commit to this branch?

@bergmeister
Copy link
Collaborator Author

Yes, please

@rjmholt
Copy link
Contributor

rjmholt commented Jan 7, 2020

Getting this:

To https://github.com/bergmeister/PSScriptAnalyzer
 ! [remote rejected] AzurePipelinesIntegration -> AzurePipelinesIntegration (permission denied)
error: failed to push some refs to 'https://github.com/bergmeister/PSScriptAnalyzer'

Going to open a PR instead

@rjmholt
Copy link
Contributor

rjmholt commented Jan 7, 2020

See bergmeister#11

Exclude failing tests on AzDO/Ubuntu
@bergmeister
Copy link
Collaborator Author

Don't know why the push did not work, especially since I had the 'Allow edits from maintainers' checkbox ticked. Thanks for the PR of the PR.

@bergmeister
Copy link
Collaborator Author

bergmeister commented Jan 7, 2020

Great, the Ubuntu build fails now because somehow after bootstrap the .net core sdk, it seems to lose it on the PATH env variable. Will merge bootstrap and build tasks into one then

@bergmeister
Copy link
Collaborator Author

Yippee, it's finally green. Now, I have to do a self-review as well as some things might've changed since then.
For example I created a build definition for each type of hosted agent image as there was a dropdown (this allowed to report them indiviudally in GitHub). Now there is only the previously chosen option left in the dropdown, which means that we could not add a new build definition with a new image. Therefore I think it might be better to put the image into YAML as well now and use only 1 build definition. What do you think?
image

inputs:
testRunner: NUnit
testResultsFiles: 'TestResults.xml'
condition: succeededOrFailed()
Copy link
Collaborator Author

@bergmeister bergmeister Jan 7, 2020

Choose a reason for hiding this comment

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

Something that should be said is that although I set it to always run, but when the build or test fails, this task does not run and therefore test results are not uploaded. This seems to be a bug with the Azure DevOps YAML though. We should raise something but I don't think that's a blocking issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

LGTM

@bergmeister bergmeister merged commit 8b2c02d into PowerShell:master Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants