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

[WIP] hide ancestry from the api #20936

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jan 7, 2021

Goal

Hide the ancestry column from the api

Why

  1. This is a private field with an internal implementation.
  2. It is easy to mess up this field if a customer edits it directly.
  3. Customers have already complained about this field.
  4. For templates, the tree is sometimes defined in this field and other times with relationships. Better for us to expose the hierarchy with the correct logic rather than have customers inferring things, possibly incorrectly.

Solution

Do not advertise this field in the API and miq expression builder.

  1. This will not prevent access to this field.
  2. Existing scripts will continue to work.
  3. Future customer implementations will be encouraged to use fields that are documented (e.g.: parent_id) and are not part of an internal implementation.

@kbrock kbrock added the api label Jan 7, 2021
Ancestry is an encoded field with the parent id.
This is only for internal consumption.

If someone asks why this was removed, we can provide alternative options
for discovering the ancestry.

I had a customer say that this field was broken because it didn't contain
something - but they weren't using the field correctly. That is why I went
on this hidden attribute journey in the first place. I wanted to limit
our exposure to customers using internal fields incorrectly.
@Fryguy
Copy link
Member

Fryguy commented Jan 7, 2021

I'm not sure about this one...it's a straight column (so it's not potentially expensive like a virtual column), and without exposing this, a user can't know the hierarchy. That is, I could see a caller using this to make a more efficient query afterwards.

@Fryguy Fryguy self-assigned this Jan 7, 2021
@kbrock
Copy link
Member Author

kbrock commented Jan 7, 2021

I propose we expose the parent_id or an array of ancestor_ids, but not the actual ancestry column with slashes

does that make more sense?

@chessbyte
Copy link
Member

@Fryguy @kbrock what should we do with this PR? Discuss, merge, close?

@kbrock
Copy link
Member Author

kbrock commented Oct 11, 2021

@chessbyte have not been able to exploit (aka change membership from one user to another owner)

But I have been able to directly modify data that I really should not have been able to do.
We don't have a clear direction moving forward so it is at the very bottom of my current and ever sprawling list.
Thinking we'll just close for now and maybe revisit in a year?

My main business case for this is I wanted to encode the ancestry as a different data type, but this exposure forces us to stick with a string encoded list of ids - which, I really want to modify in ancestry itself to something like an array of ids.

@chessbyte
Copy link
Member

I don't like closing PRs just because they are not getting attention. When the time is right, let's discuss the details of this with @Fryguy and others.

@miq-bot miq-bot added the stale label Feb 27, 2023
@miq-bot
Copy link
Member

miq-bot commented Feb 27, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@miq-bot miq-bot closed this May 29, 2023
@miq-bot
Copy link
Member

miq-bot commented May 29, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@kbrock kbrock changed the title hide ancestry from the api [WIP] hide ancestry from the api Jul 27, 2023
@kbrock kbrock removed the stale label Jul 27, 2023
@kbrock
Copy link
Member Author

kbrock commented Jul 27, 2023

WIP: need a good way to pass the ancestry to and from the ui.
Not positive that the ui actually needs the ancestry value. Maybe parent_id would be good or an array of ancestor_ids

@kbrock kbrock reopened this Jul 27, 2023
@kbrock kbrock added the wip label Jul 27, 2023
@miq-bot
Copy link
Member

miq-bot commented Jul 27, 2023

Checked commit kbrock@8bc1b25 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
5 files checked, 0 offenses detected
Everything looks fine. 🏆

@miq-bot miq-bot added the stale label Oct 30, 2023
@miq-bot
Copy link
Member

miq-bot commented Oct 30, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@miq-bot
Copy link
Member

miq-bot commented Feb 5, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

2 similar comments
@miq-bot
Copy link
Member

miq-bot commented May 6, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot
Copy link
Member

miq-bot commented Aug 12, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

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

Successfully merging this pull request may close these issues.

5 participants