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

Fixed: Made object id optional #1166

Merged
merged 7 commits into from
Aug 5, 2021
Merged

Fixed: Made object id optional #1166

merged 7 commits into from
Aug 5, 2021

Conversation

roshan-sy
Copy link
Contributor

Please make sure the code is following contribution guidelines in CONTRIBUTING.md

  • : This PR has a corresponding issue open in the Repository.
  • : Approach is signed off on the issue.

Fixes: #1132

Made object id optional while deleting the refs (i.e. heads and tags)

Changes done:

  1. Flow: Object id is resolved on the go and provided it to API, As Object Id is not optional in API.
  2. Code: E2E test case added specifically for delete ref (Create and delete ref)

@roshan-sy roshan-sy added the Artirfacts-Core The bugs under artifacts core team. label Aug 3, 2021
Copy link
Contributor

@gauravsaralMs gauravsaralMs left a comment

Choose a reason for hiding this comment

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

I would suggest adding some UTs also, specially for negative cases

Comment on lines 79 to 80
logger.error('ref not found')
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.error('ref not found')
return
raise CLIError("Failed to find object_id for ref " + name + ". Please provide object_id.")

ref = client.get_refs(repository_id=repository,
project=project,
filter=name)
if not ref:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if not ref:
if not ref or len(ref) > 1:

Copy link
Contributor

Choose a reason for hiding this comment

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

just to be sure we are not getting multiple id's from service because in that case we can't be sure which one to delete

@roshan-sy
Copy link
Contributor Author

Working on Adding more Unit test cases

@gauravsaralMs
Copy link
Contributor

@roshan-sy : unit tests are present in https://github.com/Azure/azure-devops-cli-extension/blob/master/azure-devops/azext_devops/test/repos/test_ref.py

btw: nice work adding E2E test cases, the reason we need unit test cases as well is that unit test cases are easier/ fast to run and can be used to validate small changes

one unit test is already present for delete_ref

def test_delete_ref(self):
response = delete_ref(name='sample_ref',
object_id='1234567890',
organization=TEST_DEVOPS_ORG_URL,
project='sample_project')
# assert
self.mock_update_refs.assert_called_once_with(project='sample_project',
ref_updates=ANY,
repository_id=None)

@roshan-sy
Copy link
Contributor Author

Got it, I'll add the UT as well,

@roshan-sy roshan-sy merged commit 1f8e66b into master Aug 5, 2021
@roshan-sy roshan-sy deleted the Users/Roshan/1132 branch August 5, 2021 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Artirfacts-Core The bugs under artifacts core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] az repos ref delete make object-id parameter optional
2 participants