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

feat: change /metadata REST path to /v1/metadata #3467

Merged
merged 4 commits into from
Oct 10, 2019

Conversation

spena
Copy link
Member

@spena spena commented Oct 3, 2019

Description

To be consistent with other Confluent platforms, the /metadata and /metadata/id paths were renamed to /v1/metadata and /v1/metadata/id.

Also, the /v1/metadata/id modified the output to this:

{
  "scope": {
    "path": [],
    "clusters": {
      "kafka-cluster": "ywqBIQ3PSU-K_5ttFkivCQ",
      "ksql-cluster": "default_"
    }
  },
  "id": ""
}

The kafka/ksql clusters ID are wrapped inside a clusters section. The path is also included, which for now is empty.

Testing done

  • Updated unit tests
  • Run manual tests

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@spena spena requested review from big-andy-coates and a team October 3, 2019 15:26
@spena spena self-assigned this Oct 3, 2019
Copy link
Contributor

@vcrfxia vcrfxia left a comment

Choose a reason for hiding this comment

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

LGTM with one question:

The path is also included, which for now is empty.

What will it be in the future?

@vcrfxia vcrfxia requested a review from a team October 4, 2019 00:56
@vcrfxia vcrfxia changed the title Change /metadata REST path to /v1/metadata feat: change /metadata REST path to /v1/metadata Oct 4, 2019
Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

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

Thanks @spena

I'm left wondering what the path and id are for. Both are empty strings. Knowing little about this change, I would of thought that the id field of the meta request would return the id of either the Ksql cluster or this specific node.

@spena
Copy link
Member Author

spena commented Oct 10, 2019

@big-andy-coates @vcrfxia I updated the PR to fix the assertion and add a couple of comments of what ID and PATH will be used in the future.

ID will be used to create a single URL string that includes the kafka & ksql cluster names which will be used by authorization services (as a CRN authority) to authorize access to this cluster. There's a plan to include this in 5.5 or later.

PATH is unsure what will be, but it seems it will contain the Cloud account organization where this cluster belongs to.

@spena
Copy link
Member Author

spena commented Oct 10, 2019

I will merge this change as it is needed for 5.4 and has 1 approval (thanks @vcrfxia). I chat with @rodesai offline before he left for PTO, and he gave me his +1. Although not official, I leave this comment in case he denies it (but @rodesai was ok, :))

@spena spena merged commit ed94895 into confluentinc:master Oct 10, 2019
@spena spena deleted the metadata_api branch October 10, 2019 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants