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

chore: Fix and Reconfigure GitHub Actions #319

Merged
merged 5 commits into from
Nov 15, 2022

Conversation

mikechu-optimizely
Copy link
Contributor

Summary

  • Fix failing lint_code_base
  • Change to use Windows image to compile SDK

Test plan

  • Verify all GitHub Actions are passing

Issues

  • FSSDK-8681

@mikechu-optimizely mikechu-optimizely changed the title feat: Fix and Reconfigure GitHub Actions #318 feat: Fix and Reconfigure GitHub Actions Nov 14, 2022
@mikechu-optimizely mikechu-optimizely marked this pull request as ready for review November 14, 2022 22:03
@mikechu-optimizely mikechu-optimizely requested a review from a team as a code owner November 14, 2022 22:03
@mikechu-optimizely
Copy link
Contributor Author

@msohailhussain Please check that I've set the correct "Required" tasks.

@mikechu-optimizely mikechu-optimizely changed the title feat: Fix and Reconfigure GitHub Actions chore: Fix and Reconfigure GitHub Actions Nov 14, 2022
- name: Setup NuGet
uses: NuGet/setup-nuget@v1
- name: Restore NuGet packages
run: nuget restore ./OptimizelySDK.Travis.sln
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please run OptimizelySDK.sln instead of OptimizelySDK.Travis.sln

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The OptimizelySDK.Travis.sln does not include the demo app OptimizelySDK.DemoApp.csproj which has build errors.

I'm guessing the demo app was not needed, so someone created the Travis version.

In later sprints, I think we should fix or remove the demo app.

It's likely safe to continue to build the Travis solution instead.

run: nuget restore ./OptimizelySDK.Travis.sln
- name: Build solution
run: msbuild /p:SignAssembly=true /p:AssemblyOriginatorKeyFile=$(pwd)/keypair.snk /p:Configuration=Release ./OptimizelySDK.Travis.sln
- name: Install NUnit Console
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move it before running msbuild. SO all setups can be seen together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. That makes sense.

I intended to only execute installations before they were needed in case of failure. eg If build failed, no need to install NUnit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Thinking more on this… I'd like to think green and code sustainably here.

By performing the installation only if and just before it's needed, we're reducing the amount of resources (albeit small) being used on a potentially unnecessary step.

If you feel strongly over this re-ordering, please DM me on Teams, and I'll roll it into another PR.

- name: Install NUnit Console
run: nuget install NUnit.Console -Version 3.15.2 -DirectDownload -OutputDirectory .
- name: Run NUnit tests
run: ./NUnit.ConsoleRunner.3.15.2\tools\nunit3-console.exe /timeout 10000 /process Separate ./OptimizelySDK.Tests/bin/Release/OptimizelySDK.Tests.dll
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to have, if you can specify each parameter what it does.
e.g. /process Separate and /timeout, or add a link to see what parameters are supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's a great idea. Yes. I'll add a comment.

id: unit_tests
run: |
sudo find . -path './OptimizelySDK*bin/Release/OptimizelySDK*.dll' -not -regex '.*Tests.*' -print0 | while IFS= read -r -d '' file; do sn -R $file ./keypair.snk; done
- name: Check on success
$sn = "C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.8 Tools\sn.exe";
Copy link
Contributor

Choose a reason for hiding this comment

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

great job. can you please find a way if there is any variable to represent base path. don't hardcode NETFX4.8 path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

I struggled to enumerate the path to copy of sn.exe. There are several copies of it on the windows-2019 image. The PowerShell to find and select only one is not clear to the reader, so I decided to hard-code the path.

I believe leaving this hard-coded poses little risk. The Microsoft's image should (in theory) not change since it's shared among so many builds.

I do think it's a great idea to move this base path to a YAML level variable, so it's prominent and if needed can be updated.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

all looks good. please address my comments and then merge the PR.

@mikechu-optimizely mikechu-optimizely merged commit be1c9d8 into master Nov 15, 2022
@mikechu-optimizely mikechu-optimizely deleted the mike/github-actions branch November 15, 2022 13:41
mnoman09 added a commit that referenced this pull request Mar 16, 2023
* chore : updated prefix of ticket-check action (#314)

(cherry picked from commit 678c6fd)

* chore: Fix and Reconfigure GitHub Actions (#319)

* Fix <return> method doc for IsFeatureEnabled (#329)

* [FSSDK-8938] chore: Updated newtonsoft.Json version and NjsonSchema version (#330)

* [FSSDK-8955] Refac: Replaced all instances of full stack except from changelog file and code files (#331)

* Replaced all instances of full stack except from changelog file and code files

Co-authored-by: Griffin Cox <[email protected]>

* [FSSDK-8955] Do not lint markdown for now

* [FSSDK-8955] Remove VALIDATE_MARKDOWN

---------

Co-authored-by: mnoman09 <[email protected]>
Co-authored-by: Griffin Cox <[email protected]>
Co-authored-by: Mike Chu <[email protected]>
(cherry picked from commit 5f16357)
(cherry picked from commit 887166e)

* [FSSDK-8955] Refac: Replaced all instances of full stack  from code comments and in nuspec except owner. (#332)

(cherry picked from commit 2a5b19c)

* [FSSDK-8955] chore: prepare for release 3.11.2 (#333)

* [FSSDK-8955] chore: prepare for release 3.11.2

* Nit fixed

---------

Co-authored-by: mnoman09 <[email protected]>
(cherry picked from commit e620c08)

* updated release date to 16th March

---------

Co-authored-by: Muhammad Shaharyar <[email protected]>
Co-authored-by: Mike Chu <[email protected]>
Co-authored-by: mnoman09 <[email protected]>
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.

2 participants