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.

Sign up for GitHub

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

Jump to bottom

core: fix incorrect join command #9862

Merged
merged 3 commits into from
Mar 14, 2022
Merged

core: fix incorrect join command #9862

merged 3 commits into from
Mar 14, 2022

Conversation

vavuthu
Copy link
Contributor

@vavuthu vavuthu commented Mar 9, 2022

fix incorrect join command

Signed-off-by: vavuthu vavuthu@redhat.com

Description of your changes:
correcting the join command and assign ACCESS_KEY and SECRET_KEY only in case
of actual run

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

subhamkrai
subhamkrai reviewed Mar 9, 2022
View reviewed changes
Copy link
Contributor

@subhamkrai subhamkrai left a comment •
edited
Loading

Choose a reason for hiding this comment

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

commitlint test if failing, instead of ceph use core or rgw in the commit title

@subhamkrai
Copy link
Contributor

overall looks good. Thanks for opening the PR @vavuthu

@vavuthu vavuthu changed the title ceph: fix incorrect join command core: fix incorrect join command Mar 9, 2022
@vavuthu vavuthu force-pushed the fix_external_script_join_cmd branch from b9dd125 to 6615dd5 Compare March 9, 2022 07:08
@subhamkrai subhamkrai requested a review from leseb March 9, 2022 07:11
leseb
leseb requested changes Mar 9, 2022
View reviewed changes
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

How many bugs is this fixing? I only see one commit but it seems to be doing more than just fixing the join.

@vavuthu
Copy link
Contributor Author

vavuthu commented Mar 9, 2022

How many bugs is this fixing? I only see one commit but it seems to be doing more than just fixing the join.

It fixes below 3 issues:

  1. correcting join command
  2. assign ACCESS_KEY and SECRET_KEY values only in case of actual run.
  3. Avoid creating json_out incase of dry-run

All these issues are surfaced as as result of fixing join issue.

@leseb
Copy link
Member

leseb commented Mar 9, 2022

How many bugs is this fixing? I only see one commit but it seems to be doing more than just fixing the join.

It fixes below 3 issues:

  1. correcting join command
  2. assign ACCESS_KEY and SECRET_KEY values only in case of actual run.
  3. Avoid creating json_out incase of dry-run

All these issues are surfaced as as result of fixing join issue.

Please decouple each with its own commit then

@vavuthu vavuthu force-pushed the fix_external_script_join_cmd branch from 6615dd5 to db6384b Compare March 9, 2022 12:12
@vavuthu
Copy link
Contributor Author

vavuthu commented Mar 9, 2022

How many bugs is this fixing? I only see one commit but it seems to be doing more than just fixing the join.

It fixes below 3 issues:

  1. correcting join command
  2. assign ACCESS_KEY and SECRET_KEY values only in case of actual run.
  3. Avoid creating json_out incase of dry-run

All these issues are surfaced as as result of fixing join issue.

Please decouple each with its own commit then

Done

leseb
leseb reviewed Mar 9, 2022
View reviewed changes
deploy/examples/create-external-cluster-resources.py
@@ -775,7 +775,7 @@ def create_rgw_admin_ops_user(self):
cmd = ['radosgw-admin', 'user', 'create', '--uid', self.EXTERNAL_RGW_ADMIN_OPS_USER_NAME, '--display-name',
'Rook RGW Admin Ops user', '--caps', 'buckets=*;users=*;usage=read;metadata=read;zone=read']
if self._arg_parser.dry_run:
return self.dry_run("ceph " + "".joing(cmd))
return self.dry_run("ceph " + " ".join(cmd))
Copy link
Member

Choose a reason for hiding this comment

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

We really need a python linter in the CI to avoid further issues like this. @subhamkrai could you please take care of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'll open the issue and assign that to myself.

leseb
leseb requested changes Mar 9, 2022
View reviewed changes
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

Almost there, fill all commit messages with a body. Right now, it's just a title. Thanks!

@vavuthu vavuthu force-pushed the fix_external_script_join_cmd branch from db6384b to 115d85d Compare March 9, 2022 14:06
@vavuthu
Copy link
Contributor Author

vavuthu commented Mar 9, 2022

Almost there, fill all commit messages with a body. Right now, it's just a title. Thanks!

Done

@subhamkrai subhamkrai mentioned this pull request Mar 10, 2022
Merged
7 tasks
leseb
leseb requested changes Mar 10, 2022
View reviewed changes
Copy link
Member

@leseb leseb left a comment

Choose a reason for hiding this comment

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

The title and the body are identical, this is not helpful. Please follow https://rook.io/docs/rook/latest/development-flow.html#commit-structure like indicated in the PR submission template.

@vavuthu
core: fix incorrect join command
ce98b17 
Fixing typo in join command
joing -> join

Signed-off-by: vavuthu <vavuthu@redhat.com>
@vavuthu
core: assign ACCESS_KEY and SECRET_KEY values only in case of actual run
149beed 
When using dry-run option with external script, function create_rgw_admin_ops_user
is not returning expected values which leads to TypeError. This fix is to append
values to out_map dictionary only in case of actual run.

Signed-off-by: vavuthu <vavuthu@redhat.com>
@vavuthu
core: avoid creating json_out incase of dry-run
b9d8df9 
When using dry-run option with external script, out_map dictionary
doesn't contain keys ( ACCESS_KEY and SECRET_KEY ) which leads to KeyError.
This fix to avoid creating json_out incase of dry-run since we will not use
json_out for dry-run.

Signed-off-by: vavuthu <vavuthu@redhat.com>
@vavuthu vavuthu force-pushed the fix_external_script_join_cmd branch from 115d85d to b9d8df9 Compare March 10, 2022 09:05
@vavuthu
Copy link
Contributor Author

vavuthu commented Mar 11, 2022

The title and the body are identical, this is not helpful. Please follow https://rook.io/docs/rook/latest/development-flow.html#commit-structure like indicated in the PR submission template.

updated

@leseb leseb merged commit c368c2a into rook:master Mar 14, 2022
@mergify mergify bot mentioned this pull request Mar 14, 2022
Merged
mergify bot added a commit that referenced this pull request Mar 14, 2022
@mergify
Merge pull request #9900 from rook/mergify/bp/release-1.8/pr-9862
5256ee2 
core: fix incorrect join command (backport #9862)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@subhamkrai subhamkrai subhamkrai approved these changes

@leseb leseb leseb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vavuthu @subhamkrai @leseb