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

Application plan import: input resource as source of truth #245

Merged
merged 4 commits into from Apr 22, 2020

Conversation

eguzki
Copy link
Member

@eguzki eguzki commented Mar 23, 2020

Application plan import command takes as source of truth the imported resource

@eguzki eguzki changed the title [WIP] application plan import: input resource as source of truth [WIP][BLOCKED] application plan import: input resource as source of truth Mar 23, 2020
@eguzki eguzki changed the title [WIP][BLOCKED] application plan import: input resource as source of truth [WIP] Application plan import: input resource as source of truth Apr 16, 2020
@eguzki eguzki changed the title [WIP] Application plan import: input resource as source of truth Application plan import: input resource as source of truth Apr 17, 2020
@eguzki
Copy link
Member Author

eguzki commented Apr 17, 2020

ready for review @miguelsorianod

@miguelsorianod
Copy link
Contributor

Can you provide some context about why do we want the behavior changed? What's the motivation of it? Is there some issue or something where the motivation is explained?

If I understand correctly, we have changed the behavior, where:

  • Before: When importing application plan, we only import the pricing rules and limits that exist in the import file that at the same time don't exist in the service where we want to import them. It's like sort of a merge behaviour, but we don't merge the ones existing in both sides.
  • Now: When importing an application plan, if there are any existing pricing rules and limits in the service we want to import to, we delete all of them and we import the ones defined in the import file. It's sort of a set behaviour, where we set what is defined in the import file side

Am I correct?

Thank you.

Copy link
Contributor

@miguelsorianod miguelsorianod left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Left a comment related to the purpose of this changes.

Additionally, I think it would be a good idea to document this behaviour change at least in the changelog, as I understand it can potentially affect users of the tool with existing scripts or automated pipelines that they might have

@eguzki
Copy link
Member Author

eguzki commented Apr 22, 2020

Can you provide some context about why do we want the behavior changed? What's the motivation of it? Is there some issue or something where the motivation is explained?

https://issues.redhat.com/browse/THREESCALE-3874
https://issues.redhat.com/browse/THREESCALE-3211

  • Before: When importing application plan, we only import the pricing rules and limits that exist in the import file that at the same time don't exist in the service where we want to import them. It's like sort of a merge behaviour, but we don't merge the ones existing in both sides.
  • Now: When importing an application plan, if there are any existing pricing rules and limits in the service we want to import to, we delete all of them and we import the ones defined in the import file. It's sort of a set behaviour, where we set what is defined in the import file side
    Am I correct?

Yes your are correct. The main implication with the change in the "merge strategy" is that with the SET semantics, existing limits and pricing rules not related with the imported file content will be deleted as well.

@eguzki eguzki assigned miguelsorianod and unassigned eguzki Apr 22, 2020
Copy link
Contributor

@miguelsorianod miguelsorianod left a comment

Choose a reason for hiding this comment

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

👍 With the issues that contain context information is much more clear thanks

@eguzki eguzki merged commit e5351b2 into master Apr 22, 2020
@eguzki eguzki deleted the THREESCALE-3874 branch April 22, 2020 11:05
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.

None yet

2 participants