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: check the error chain when validating if it's x509 error #3175

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

lubronzhan
Copy link
Contributor

@lubronzhan lubronzhan commented Jul 10, 2023

Description

Please include a summary of the change and which issue is fixed. Please also
include relevant motivation and context. List any dependencies that are required
for this change.

Check the error chain when checking if error is x509 related. This should be compatible with both golang 1.20 and 1.19

Closes: ##3174

Type of change

Please mark options that are relevant:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update
  • Build related change

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. If applicable, please also list any relevant
details for your test configuration.

  • Test Description 1
    Compiles CPI with 1.20 and configures CPI with tlsthumbprint, and the error doesn't appear.
  • Test Description 2

Checklist:

  • My code follows the CONTRIBUTION
    guidelines of
    this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

@sbueringer
Copy link

sbueringer commented Jul 13, 2023

cc @dougm

(context: this issue currently blocks adoption of Go 1.20 in CAPV)

@lubronzhan
Copy link
Contributor Author

I think Doug is on PTO this week, he should be able to review this next week

@sbueringer
Copy link

Perfect!

@lubronzhan lubronzhan requested a review from dougm July 17, 2023 17:00
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Thanks @lubronzhan
The commit message is used to generate change log, auto-close issues, etc., can you please update? Currently is:

issue-3174: check the error chain when validating if it's
 x509 error

Should include fix: prefix and Closes: #NNNN:

fix: check the error chain when validating x509 error

Closes: #3174

vim25/soap/error.go Outdated Show resolved Hide resolved
@lubronzhan lubronzhan force-pushed the issue-3174 branch 2 times, most recently from 2bd147a to 04f8594 Compare July 17, 2023 17:49
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Thanks @lubronzhan
We can merge when CI passes (snapshot release failed, sometimes will timeout)

@sbueringer
Copy link

@dougm Newbie question (entirely new to CAPV / govmomi). How quickly could we get a release which includes this fix? :)

@lubronzhan lubronzhan merged commit e2915da into vmware:main Jul 17, 2023
10 checks passed
@lubronzhan lubronzhan deleted the issue-3174 branch July 17, 2023 18:44
@dougm
Copy link
Member

dougm commented Jul 17, 2023

@dougm Newbie question (entirely new to CAPV / govmomi). How quickly could we get a release which includes this fix? :)

I cherry-picked the change to the release-0.30 branch and started a release workflow. Normally takes a few minutes, but goreleaser failed: https://github.com/vmware/govmomi/actions/runs/5579622016/jobs/10195422283

I haven't looked close yet, but goreleaser has been removing deprecated stuff recently. So there's no govc release binary yet, but the new v0.30.6 tag is there that CAPV can use: https://github.com/vmware/govmomi/tree/v0.30.6

@sbueringer
Copy link

Perfect.

Thank you very much!!

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.

4 participants