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

radosgw-admin zone deletion uses zone delete #44989

Merged
merged 1 commit into from Apr 25, 2022
Merged

Conversation

Ejdesgaard
Copy link
Contributor

@Ejdesgaard Ejdesgaard commented Feb 11, 2022

doc/radosgw/multisite.rst: zone rm does not work on pacific, zone delete works

@Ejdesgaard Ejdesgaard requested a review from a team as a code owner February 11, 2022 10:47
@ljflores
Copy link
Contributor

Hey @Ejdesgaard, looks like your commits need a "Signed-off-by" signature: https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#developer-s-certificate-of-origin-1-1

@Ejdesgaard
Copy link
Contributor Author

Hey @Ejdesgaard, looks like your commits need a "Signed-off-by" signature: https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#developer-s-certificate-of-origin-1-1

I have setup git on my computer to sign-off all commits, moving forward, but I can't figure out how I can amend the 2 commits such that they get signed.
Do I need to drop the commits and make the changes again and commit them signed, or is there a way for me to sign the existing commits ?

@anthonyeleven
Copy link
Contributor

I am far from a git expert, but what I’ve done in the past to resolve this :guilty: has been git commit -amend followed by a force push.

@Ejdesgaard
Copy link
Contributor Author

I am far from a git expert, but what I’ve done in the past to resolve this :guilty: has been git commit -amend followed by a force push.

I can only get git commit --amend to work on HEAD and I need to do it on 2 commits that are far from the current HEAD
Is there a way to specify a commit id to --amend ?

@anthonyeleven
Copy link
Contributor

Maybe squash them and edit the commit message as part of the process? It’s common for git projects to prefer PRs to have a single commit anyway.

@ljflores
Copy link
Contributor

@Ejdesgaard try this set of commands:

  • git log
  • git rebase -i <SHA of the commit right before your two commits>
  • Change pick to edit in front of the two commits
  • git commit --amend (this edits the first commit)
  • Make your edits and save the commit
  • git rebase --continue (this moves to the second commit)
  • git commit --amend (edits the second commit)
  • Make your edits and save the commit
  • git rebase --continue (completes the rebase of both commits)

If done successfully, you should see a message something like Successfully rebased and updated refs/heads/master..

  • Run git log and make sure that the commits are how you want them.
  • git push -f <ceph repository> <branch name> (you have to force-push the branch now since you changed the log history)

As you are making your edits, I noticed that in addition to adding a "Signed-off-by" line, you also should prefix your commits with the directory the changes belong to, in your case doc/radosgw. See this document for more details: https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst (it explains how you should sign off your commits and title them).

@ljflores ljflores self-requested a review March 23, 2022 14:35
doc/radosgw: radosgw-admin zonegroup rm changed to radosgw-admin zonegroup delete

Signed-off-by: Heðin Ejdesgaard <hej@ejdesgaard.fo>
@Ejdesgaard
Copy link
Contributor Author

Ejdesgaard commented Mar 23, 2022

@ljflores
That solved it :D
I also merged the commits into 1 commit and found another instance of zone rm that had to be amended.

I have also tried to find out when it was changed and with git checkout [v16.2.7|v15.2.7|v14.2.7] and for each of them git blame src/rgw/rgw_admin.cc |grep OPT::ZONE_DELETE it seems rm was replaced with delete between v14 and v15.

@ljflores ljflores requested a review from cbodley March 23, 2022 18:56
Copy link
Contributor

@ljflores ljflores 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. @cbodley I re-requested your review since there was another change made to the docs.

@Ejdesgaard
Copy link
Contributor Author

Do I need to do any modifications in order to get this merged and back-ported ?

@ljflores
Copy link
Contributor

@ljflores That solved it :D I also merged the commits into 1 commit and found another instance of zone rm that had to be amended.

I have also tried to find out when it was changed and with git checkout [v16.2.7|v15.2.7|v14.2.7] and for each of them git blame src/rgw/rgw_admin.cc |grep OPT::ZONE_DELETE it seems rm was replaced with delete between v14 and v15.

@Ejdesgaard just need re-approval from @cbodley or another RGW person based on your new changes. Once there's a final approval, it will be okay to merge.

@cbodley
Copy link
Contributor

cbodley commented Apr 25, 2022

if this needs backporting, is there a corresponding tracker issue for this?

@Ejdesgaard
Copy link
Contributor Author

if this needs backporting, is there a corresponding tracker issue for this?

I'm afraid not...
I'm not sure what the workflow for this is.

I would be happy to give it a go if you can point me at some documentation on how to do it :)

@cbodley cbodley merged commit a9cb584 into ceph:master Apr 25, 2022
@cbodley
Copy link
Contributor

cbodley commented Apr 25, 2022

I would be happy to give it a go if you can point me at some documentation on how to do it :)

@Ejdesgaard no worries, i opened https://tracker.ceph.com/issues/55434 against pacific/quincy. thanks!

@ljflores thanks for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants