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 windows signing #24940

Merged
merged 1 commit into from
Nov 10, 2023
Merged

fix windows signing #24940

merged 1 commit into from
Nov 10, 2023

Conversation

kisantia
Copy link
Contributor

Addresses #24930. This reverts the changes to sql-product-build-win32.yml in #21311. There no longer seems to be the issue with signing failing for .node files, and for some reason the minimatch pattern was not finding all the expected files.

We'll need to keep an eye on this in case there are signing issues again due to the .node files, but vscode uses a pattern include that includes *.node (although using a differen task), so I'm less concerned about that.

Copy link
Contributor

@Charles-Gagnon Charles-Gagnon left a comment

Choose a reason for hiding this comment

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

In general we should try to do the same thing that VS Code is doing, so I'd be interested to know when they switched over to what they're currently using and why (unless they were using it from the start - and then my question would be why didn't we do the same)

I definitely have some concern that any differences like this could result in similar issues down the line if things are changing again...

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6827935698

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 41.57%

Files with Coverage Reduction New Missed Lines %
extensions/notebook/src/book/bookTreeView.ts 1 36.91%
Totals Coverage Status
Change from base Build 6819183022: 0.0%
Covered Lines: 30785
Relevant Lines: 69355

💛 - Coveralls

@kisantia
Copy link
Contributor Author

In general we should try to do the same thing that VS Code is doing, so I'd be interested to know when they switched over to what they're currently using and why (unless they were using it from the start - and then my question would be why didn't we do the same)

I definitely have some concern that any differences like this could result in similar issues down the line if things are changing again...

yup that's something I'll look more into when I'm DRI next week!

@kisantia
Copy link
Contributor Author

merging now that windows checks are finished. The mac failure is at the install dependencies step that's been failing intermittently (will investigate more later), but that step is passing in the mac smoke tests check.

@kisantia kisantia merged commit 8a6a18f into main Nov 10, 2023
14 of 19 checks passed
@kisantia kisantia deleted the kisantia/fixSigning branch November 10, 2023 19:03
kisantia added a commit that referenced this pull request Nov 10, 2023
siyao-Siyang pushed a commit that referenced this pull request Nov 13, 2023
@kisantia kisantia mentioned this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants