Skip to content

Fixed a bug where key value pair was not returning data correctly when get_by_name function is called. #5677

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

Merged
merged 11 commits into from
Aug 24, 2022

Conversation

bharath-orchestral
Copy link
Contributor

Bug: Calling 'get_by_name' on client for getting key details was not returning any results despite key being stored. Following is the sample code for replicating the bug

from st2common.runners.base_action import Action
from st2client.client import Client

class actionContext(Action):
    def run(self):

        client = Client(base_url='http://localhost')
        result = client.keys.get_by_name(name='test_key',scope='user')
        self.logger.debug(result)
        return result

Resolution: 'keys' property in Client is of type 'ResourceManager'. We created a new class 'KeyValuePairResourceManager' which inherits from 'ResourceManager'. We assigned 'KeyValuePairResourceManager' to 'keys' property. We have created get_by_name in this class. KeyValuePairResourceManager's get_by_name calls "/keys/{keyname}" url while the old function used to call '/keys' url.
We looked into test cases for classes which inherit from 'ResourceManager' but did not find any. There are test cases
for functions in 'ResourceManager' but not for classes inheriting from it.
But we wrote a unit test case which tests KeyValuePairResourceManager's get_by_name function

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Jul 19, 2022
@bharath-orchestral
Copy link
Contributor Author

Resubmitted the PR with correct credentials.

@bharath-orchestral
Copy link
Contributor Author

Can anyone help me on how to add changes to CHANGELOG.rst? Do I need to run some command like make lint?

@cognifloyd
Copy link
Member

@bharath-orchestral Thanks for working on this!

Here is the changelog: https://github.com/StackStorm/st2/blob/master/CHANGELOG.rst
There isn't a command that auto-generates a changelog entry (though I wish we had that).
Here is an example of a adding a changelog entry: 8e908a5

For this PR, you will need to put your changelog entry under In Development (at the top of the file) at the end of the list of Fixed issues.

@cognifloyd
Copy link
Member

To run the black formatter you can use make black.

@cognifloyd cognifloyd added this to the 3.8.0 milestone Jul 19, 2022
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Please run black on the files you've changed.

Also, I wonder if the code duplication can be reduced.

if response.status_code != http_client.OK:
self.handle_error(response)

return self.resource.deserialize(parse_api_response(response))
Copy link
Member

Choose a reason for hiding this comment

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

needs new line at EOF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

resource = mgr.get_by_name("abc")
actual = resource.serialize()
expected = json.loads(json.dumps(base.RESOURCES[0]))
self.assertEqual(actual, expected)
Copy link
Member

Choose a reason for hiding this comment

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

new line at EOF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@cognifloyd
Copy link
Member

Deleting that init.py file also removed the folders it was in, so now there are failures in some unit tests. Could you restore that please? Or a .gitkeep file would probably work

@bharath-orchestral
Copy link
Contributor Author

I added that init.py in that folder but for some reason orquesta CI/ Integration Tests are failing. @cognifloyd Do you know the reason for this?

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Flaky test. Oh, sand I just noticed the header in that empty __init__.py. please don't add headers to empty __init__.py files like that.

@@ -0,0 +1,14 @@
# Copyright 2020 The StackStorm Authors.
Copy link
Member

Choose a reason for hiding this comment

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

__init__.py should be empty unless there is logic in the file. Please delete all these headers.

Copy link
Contributor Author

@bharath-orchestral bharath-orchestral Jul 26, 2022

Choose a reason for hiding this comment

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

Deleted all the headers. Now __init__.py is empty

Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one really small comment on the changelog

@m4dcoder m4dcoder enabled auto-merge August 24, 2022 19:56
@m4dcoder m4dcoder merged commit 85e4066 into StackStorm:master Aug 24, 2022
@m4dcoder m4dcoder deleted the issue/get-key-value-by-name branch August 24, 2022 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M PR that changes 30-99 lines. Good size to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants