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(migrations): SpelLoadBalancersMigration runs on every migration #1070

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

Conversation

JonGilmore
Copy link

isValid returns true for SpelLoadBalancersMigration and there is no VALID_UNTIL set. Because of this, this migration runs regardless of the migration that were planning to run. I imagine the intent was originally to run this once, but in its current configuration, that is not true.

`isValid` returns true for `SpelLoadBalancersMigration` and there is no `VALID_UNTIL` set. Because of this, this migration runs regardless of the migration that were planning to run. I imagine the intent was originally to run this once, but in its current configuration, that is not true.
@JonGilmore
Copy link
Author

tagging @ajordens as it appears you added this in #1025

import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Component;

@Component
@Slf4j
public class SpelLoadBalancersMigration implements Migration {
// Only valid until October 13th, 2021
private static final Date VALID_UNTIL = new GregorianCalendar(2021, 10, 13).getTime();
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I'm being dense, what's the significance of this date?

Copy link
Author

Choose a reason for hiding this comment

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

No apologies necessary, it was just the day before I opened this PR, no significance at all really.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with migrations, is the intent that this migration should never run after this PR is merged and released? Is there a way to explicitly enable it after this PR?

Trying to understand if people on older versions of Spinnaker may have issues upgrading if this migration is skipped.

Copy link
Author

@JonGilmore JonGilmore Dec 4, 2021

Choose a reason for hiding this comment

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

If you look at some other examples of how Netflix has done migrations, they usually do them 1 of 2 ways - the way that I've done it with a specific date or with a ConditionalOnProperty. The benefit to doing it with a date like this is that you don't have to put up another PR to your config after you're done running the migration to disable it.

Trying to understand if people on older versions of Spinnaker may have issues upgrading if this migration is skipped.

That's a good question, this migration didn't actually do anything for us because we didn't have any pipelines (out of our thousands that we have) that met this condition.
Here are the PRs that should give a little more background on what the intent of this migration was for: #1025 and spinnaker/deck#8957

Copy link
Author

Choose a reason for hiding this comment

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

Here's an example of an alternative way that migrations have been done

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll have to wait for @ajordens verdict - I'm not sure if this is a minor issue or a migration everybody needs to do.

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.

2 participants