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

Fix dependencies for server-side deploy script #10604

Merged
merged 3 commits into from Dec 13, 2019
Merged

Fix dependencies for server-side deploy script #10604

merged 3 commits into from Dec 13, 2019

Conversation

@feerrenrut
Copy link
Contributor

feerrenrut commented Dec 10, 2019

Link to issue number:

Summary of the issue:

The server side deploy code ('nvdaAppveyorHook') relies on artifacts, they must be uploaded before the deploy step is run.

deploy_script is run before on_finish, so the artifacts were missing.

Description of how this pull request fixes the issue:

Instead, use artifacts section, and also upload from on_failure.

For ordering of appveyor yml phases, see: https://www.appveyor.com/docs/build-configuration/#build-pipeline

Testing performed:

Known issues with pull request:

Change log entry:

Section: New features, Changes, Bug fixes

@AppVeyorBot

This comment was marked as outdated.

Copy link

AppVeyorBot commented Dec 10, 2019

@AppVeyorBot

This comment was marked as outdated.

Copy link

AppVeyorBot commented Dec 10, 2019

See test results for failed build of commit 7c04477b76

@AppVeyorBot

This comment was marked as outdated.

Copy link

AppVeyorBot commented Dec 10, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Dec 10, 2019

@AppVeyorBot

This comment was marked as outdated.

Copy link

AppVeyorBot commented Dec 10, 2019

@feerrenrut feerrenrut marked this pull request as ready for review Dec 10, 2019
@feerrenrut feerrenrut requested a review from michaelDCurran Dec 10, 2019
@feerrenrut

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Dec 10, 2019

@michaelDCurran This should be ready to merge. It should fix the issues with the deploy process. There are also now messages for unit / system test failures, and direct link to the PR build executable.

@AppVeyorBot

This comment has been minimized.

Copy link
Contributor

michaelDCurran left a comment

This all looks good to me, and based on appveyor documentation should work as we expect. However, we won't truely know until we try to make beta2.

$exe = Get-ChildItem -Name output\*.exe
if($?){
$exeUrl="$appVeyorUrl/api/buildjobs/$env:APPVEYOR_JOB_ID/artifacts/output/$exe"
Add-AppveyorMessage "[Build (for testing PR)]($exeUrl)"

This comment has been minimized.

Copy link
@michaelDCurran

michaelDCurran Dec 10, 2019

Contributor

As much as I get the logic, I feel this does get a bit too noisy. Especially as it will send an email to anyone associated with the pr, and at first glance looks like the build failed. I'd prefer for now not to include this final part.

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 11, 2019

Author Contributor

I guess what you mean is that you don't want any comment on successful builds. This is configured in the appveyor settings for the project, along with a template for how these "messages" are formatted. It is in the "notifications" section. I can take this out for now until we discuss it further, but incidentally I've noticed PR's get more testing when a link to the build is easy to access. The process of manually pasting the link in is annoying and easy to forget.

It would be easy to adjust the template to start with a "Success!" as a heading or whatever. For now I'll disable messages on successful builds.

This comment has been minimized.

Copy link
@feerrenrut

feerrenrut Dec 11, 2019

Author Contributor

For the last two messages from appveyor(success and failure), the notification template looked like this:

{{#jobs}}
{{#messages}}
- {{message}}
{{/messages}}
{{/jobs}}


{{#failed}}
See [test results]({{buildUrl}}/tests) for [{{status}} build]({{buildUrl}}) of [commit {{commitId}}]({{commitUrl}})
{{/failed}}


{{#passed}}
See [{{status}} build]({{buildUrl}}) of [commit {{commitId}}]({{commitUrl}})
{{/passed}}

This is a "Mustache template" the appveyor docs in the "customizing message template" section have some examples, and a link to a more formal description of the format. On the same page, but further up there is a description of the available variables in the "webhook payload" section.

@feerrenrut feerrenrut changed the base branch from master to beta Dec 13, 2019
feerrenrut added 3 commits Dec 10, 2019
The server side deploy code ('nvdaAppveyorHook') relies on artifacts,
they must be uploaded first.

deploy_script is run before on_finish, so the artifacts were missing.
Instead, use artifacts section, and also upload from on_failure.

For ordering of appveyor yml phases,
see: https://www.appveyor.com/docs/build-configuration/#build-pipeline
We know that the executable is present, since the name comes from the
'output' directory of the build. Though this depends on the artifact
uploading being successful.
Adds a message for:
- Unable to install NVDA
- Unit test failure
- System test failure
@feerrenrut feerrenrut force-pushed the fixDeployStep branch from 1ea363a to bdab321 Dec 13, 2019
@feerrenrut

This comment has been minimized.

Copy link
Contributor Author

feerrenrut commented Dec 13, 2019

I have changed the target of this PR to Beta, and rebased the changes. Assuming a successful CI build, I believe we can merge it.

@feerrenrut feerrenrut merged commit db0b200 into beta Dec 13, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Dec 13, 2019
@feerrenrut feerrenrut deleted the fixDeployStep branch Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.