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

bugfix: Add GetIsDeleted() check to Azure DevOps Event Handler #2838

Merged
merged 5 commits into from
Dec 22, 2022

Conversation

SSKLCP
Copy link
Contributor

@SSKLCP SSKLCP commented Dec 20, 2022

what

Changing property check for isDeleted to ADO function GetIsDeleted() as that handles when the property doesn't exist

why

Issues when IsDeleted doesn't exist.

references

@SSKLCP SSKLCP requested a review from a team as a code owner December 20, 2022 08:52
@jsw1993
Copy link
Contributor

jsw1993 commented Dec 20, 2022

Is it possible to add a test for a comment without the IsDeleted field?

@SSKLCP
Copy link
Contributor Author

SSKLCP commented Dec 20, 2022

@jsw1993 - See the attempted fix for this.

Ideally I wanted a test to say "If it's a valid request then pass" but because of the way it works, it tries to go to the repo URL to get the data and I'm not sure there is a valid repo to point at to test.
The line is here:

baseRepo, err := e.Parser.ParseAzureDevopsRepo(resource.PullRequest.GetRepository())

Instead I've added a test which passes if it gets to (and fails) at the parsing section so at least we know that it's passed the ignore conditions.
If you have any better ideas on how to handle it let me know and I'm happy to implement

@jsw1993
Copy link
Contributor

jsw1993 commented Dec 20, 2022

Sounds good to me but I'll let someone more versed in Go provide a proper judgement. Many thanks.

@nitrocode
Copy link
Member

Thank you @SSKLCP for the quick fix!

@nitrocode nitrocode added the needs tests Change requires tests label Dec 21, 2022
Copy link
Contributor

@krrrr38 krrrr38 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@nitrocode
Copy link
Member

@SSKLCP since #2850 is merged, do we need any changes in this pr or are we good to merge?

@SSKLCP
Copy link
Contributor Author

SSKLCP commented Dec 22, 2022

@nitrocode - I've just changed the test to match #2850's testing pattern but other than that I'm happy so long as others are.
Thanks 😃

Copy link
Member

@nitrocode nitrocode left a comment

Choose a reason for hiding this comment

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

Nice work!

@nitrocode nitrocode merged commit 01ce8ce into runatlantis:main Dec 22, 2022
@nitrocode nitrocode added this to the 0.22.0 milestone Dec 23, 2022
@nitrocode nitrocode changed the title Adding GetIsDeleted() check to Azure DevOps Event Handler bugfix: Add GetIsDeleted() check to Azure DevOps Event Handler Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Change requires tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Atlantis panics on Azure DevOps comment when using v0.22.0-pre.20221219
4 participants