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

fix: create and delete feed changes #3476

Merged
merged 2 commits into from
May 24, 2023
Merged

Conversation

ainouzgali
Copy link
Contributor

What change does this PR introduce?

create and then delete feed creates two changes to promote, depending on the order of promotion the deleted feed will be created in prod with no option of deleting it.

Why was this change needed?

Other information (Screenshots)

@linear
Copy link

linear bot commented May 23, 2023

NV-2347 Create Feed changes tests

Create and then delete feed creates two changes to promote, depending on the order of promotion the deleted feed will be created in prod with no option of deleting it.

@ainouzgali ainouzgali changed the base branch from next to nv-2340-variable-values-not-promoted-to-prod May 23, 2023 10:08
Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

I have one concern about the logic.

Comment on lines +20 to +22
if (command.item.deleted) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it doesn't make sense to have it here. If the item passed in the command has the deleted property, it will come already so we can check it and do the early return before even we do the repository call, saving one hit to the database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it is to handle the case of a promotion of a delete that does not yet exists in prod. Meaning -> created, deleted and only then promoted.
If the first call finds it in prod(created->promoted->deleted->promoted), it will not get to this check and will delete it from prod at the end of this file. If doesn't find it in prod and not deleted now -> will create it.

Copy link
Contributor

Choose a reason for hiding this comment

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

But for the case you mention I would expect item.deleted to not be set. If it is passed I think something would be very wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why wouldn't it be set ? I don't think I follow @p-fernandez

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that item is the prod feed and command.item is the dev feed.

Say a feed was created in dev, then deleted in dev. Then promoted.

The command.item is the aggregated item of all changes in dev-> which means that command.item should also have the deleted flag.

It looks for the feed in prod, which doesn't exist - as it was never created. And then if the promotion is of a feed that was deleted anyway - there is no reason to do anything for prod env.

But I might be missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, this changes things a bit. I would rename item to productionItem as from the variables we have no way to know that we are looking for an item in production.

Base automatically changed from nv-2340-variable-values-not-promoted-to-prod to next May 24, 2023 09:18
Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

🌟

Comment on lines +20 to +22
if (command.item.deleted) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, this changes things a bit. I would rename item to productionItem as from the variables we have no way to know that we are looking for an item in production.

@ainouzgali ainouzgali added this pull request to the merge queue May 24, 2023
Merged via the queue into next with commit da3a175 May 24, 2023
19 checks passed
@ainouzgali ainouzgali deleted the nv-2347-create-feed-changes-tests branch May 24, 2023 10:36
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.

None yet

2 participants