Skip to content

mds: use variable g_ceph_context directly in MDSAuthCaps#50874

Merged
rishabh-d-dave merged 1 commit intoceph:mainfrom
rishabh-d-dave:mds-log-variable
Jun 16, 2023
Merged

mds: use variable g_ceph_context directly in MDSAuthCaps#50874
rishabh-d-dave merged 1 commit intoceph:mainfrom
rishabh-d-dave:mds-log-variable

Conversation

@rishabh-d-dave
Copy link
Contributor

Variable g_ceph_context is global, therefore use it directly instead of
passing it as a parameter to method.

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
    • Code improvement
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@dparmar18
Copy link
Contributor

the part of code looks much cleaner with this change

Copy link
Contributor

@dparmar18 dparmar18 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

@vshankar
Copy link
Contributor

@rishabh-d-dave please pick this up in your next run.

@rishabh-d-dave
Copy link
Contributor Author

@vshankar

@rishabh-d-dave please pick this up in your next run.

Yes.

@rishabh-d-dave
Copy link
Contributor Author

No changes, just a rebase.

Copy link
Contributor

@rzarzynski rzarzynski left a comment

Choose a reason for hiding this comment

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

The tiny mon change LGTM.
I hasn't checked the implicit assumption MDSAuthCaps isn't / won't be used in libraries.

} else if (type == "mds") {
MDSAuthCaps mdscap;
if (!mdscap.parse(g_ceph_context, caps, out)) {
if (!mdscap.parse(caps, out)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

From the mon's perspective this should be fine.

Just a note:

It is fine to use dout and derr in daemons, but in library code, you must use ldout and lderr, and pass in your own CephContext object. The compiler will enforce this restriction.

Source: https://docs.ceph.com/en/latest/dev/context/

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@vshankar
Copy link
Contributor

@rishabh-d-dave please rebase and do the needful.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jun 15, 2023

@vshankar

@rishabh-d-dave please rebase and do the needful.

Yes, I am on it. Due to teuthology infra issues, testing wasn't complete but it is now. Will review results once more and merge this PR.

Copy link
Contributor Author

@rishabh-d-dave rishabh-d-dave left a comment

Choose a reason for hiding this comment

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

Variable g_ceph_context is global, therefore use it directly instead of
passing it as a parameter to method.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave rishabh-d-dave merged commit 1961bf8 into ceph:main Jun 16, 2023
rishabh-d-dave added a commit to rishabh-d-dave/ceph that referenced this pull request Jun 16, 2023
Since PR ceph#50874 has been merged, this method can do the exact same job
in much lesser code.

Also since we are here, let's rename variable "type" to "entity".

Signed-off-by: Rishabh Dave <ridave@redhat.com>
rishabh-d-dave added a commit to rishabh-d-dave/ceph that referenced this pull request Jun 18, 2023
Since PR ceph#50874 has been merged, this method can do the exact same job
in much lesser code.

Also since we are here, let's rename variable "type" to "entity".

Signed-off-by: Rishabh Dave <ridave@redhat.com>
rishabh-d-dave added a commit to rishabh-d-dave/ceph that referenced this pull request Jun 18, 2023
Since PR ceph#50874 has been merged, this method can do the exact same job
in much lesser code.

Also since we are here, let's rename variable "type" to "entity".

Signed-off-by: Rishabh Dave <ridave@redhat.com>
rishabh-d-dave added a commit to rishabh-d-dave/ceph that referenced this pull request Jun 19, 2023
Since PR ceph#50874 has been merged, this method can do the exact same job
in much lesser code.

Also since we are here, let's rename variable "type" to "entity".

Signed-off-by: Rishabh Dave <ridave@redhat.com>
joscollin pushed a commit to joscollin/ceph that referenced this pull request Jul 31, 2023
Since PR ceph#50874 has been merged, this method can do the exact same job
in much lesser code.

Also since we are here, let's rename variable "type" to "entity".

Signed-off-by: Rishabh Dave <ridave@redhat.com>
aaSharma14 pushed a commit to rhcs-dashboard/ceph that referenced this pull request Jan 2, 2024
Since PR ceph#50874 has been merged, this method can do the exact same job
in much lesser code.

Also since we are here, let's rename variable "type" to "entity".

Signed-off-by: Rishabh Dave <ridave@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants