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

SALTO-7225: Change the elemID of dashboards to contain the user id #7212

Closed
wants to merge 2 commits into from

Conversation

YuvalGivon
Copy link
Contributor

@YuvalGivon YuvalGivon commented Feb 6, 2025

Added idFields to generate an elemId composed of the dashboard name and the owner's name.


Release Notes:
Jira_adapter:

  • Dashboard elemIDs will now be determined by a combination of their name and owner ID.

User Notifications:
Jira_adapter:

  • Dashboard elemIDs will now be determined by a combination of their name and owner ID.

@YuvalGivon YuvalGivon marked this pull request as draft February 6, 2025 12:54
@coveralls
Copy link

coveralls commented Feb 6, 2025

Coverage Status

coverage: 93.635%. remained the same
when pulling 103b338 on YuvalGivon:SALTO-7225
into fab89c8 on salto-io:main.

@YuvalGivon YuvalGivon marked this pull request as ready for review February 9, 2025 16:54
Copy link
Contributor

@shir-reifenberg shir-reifenberg left a comment

Choose a reason for hiding this comment

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

Wouldn't this cause significant diffs across environments ? do we expect users to be aligned between envs ?
Did we consider doing this only when there is a collision ?

@YuvalGivon
Copy link
Contributor Author

YuvalGivon commented Feb 10, 2025

Wouldn't this cause significant diffs across environments ? do we expect users to be aligned between envs ? Did we consider doing this only when there is a collision ?

It is, I'm going to merge it only after i will get a confirmation from CS, users will need to run a fetch with regenerateSaltoIds

@shir-reifenberg
Copy link
Contributor

Wouldn't this cause significant diffs across environments ? do we expect users to be aligned between envs ? Did we consider doing this only when there is a collision ?

It is, I'm going to merge it only after i will get a confirmation from CS, users will need to run a fetch with regenerateSaltoIds

I'm really not in the details of this and i'm not sure what research was done, but misaligned ids can cause lots of other issues. If it's really the only choice, I would consider the option to "fallback" to user id in the element id, only in the case of a collision.
Can also discuss IRL 😄

@IdoZakk
Copy link
Contributor

IdoZakk commented Feb 20, 2025

Wouldn't this cause significant diffs across environments ? do we expect users to be aligned between envs ? Did we consider doing this only when there is a collision ?

It is, I'm going to merge it only after i will get a confirmation from CS, users will need to run a fetch with regenerateSaltoIds

I'm really not in the details of this and i'm not sure what research was done, but misaligned ids can cause lots of other issues. If it's really the only choice, I would consider the option to "fallback" to user id in the element id, only in the case of a collision. Can also discuss IRL 😄

I think fallback will result in more collisions.
The dashboard's uniqueness in Jira is based on name and owner, so there are many dashboards we do not bring. The account id is the same across environments.
The problem of misaligned ids will happen in two cases- two existing envs where there is only collision in one env, and comparing an existing env to a new env. We can cover them with a CV.

There is a problem around deployments- we cannot deploy the owner of a dashboard, so deploying a dashboard made by another user will still appear as a difference after a deployment.

I think the current situation of collisions is not a good UX.
I see 3 main alternative options:

  1. Add owner to the id. We should add a CV, and also a post action for dashboard deployment to replace the owner
  2. Same as number 1, but change the owner of the dashboard using private API (simple implementation)
  3. Fetch only the dashboards of the credentials' user

I think 1 is the best value/time, and if customers start to experience issues because they did not perform the post action we can implement 2.
@kami-delaroz @shir-reifenberg WDYT?

@kami-delaroz
Copy link
Contributor

@IdoZakk we're planning to apply this change to the customers who got this collision, and instruct them to fetch with regenerate ids on all their envs

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.

5 participants