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.

Sign up for GitHub

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

Jump to bottom

fix(migrations): SpelLoadBalancersMigration runs on every migration #1070

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

fix(migrations): SpelLoadBalancersMigration runs on every migration #1070

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions ...ns/src/main/java/com/netflix/spinnaker/front50/migrations/SpelLoadBalancersMigration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,28 @@

import com.netflix.spinnaker.front50.api.model.pipeline.Pipeline;
import com.netflix.spinnaker.front50.model.pipeline.PipelineDAO;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.time.Clock;
import java.util.*;
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 •
edited
Loading

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.

private Clock clock = Clock.systemDefaultZone();

private final PipelineDAO pipelineDAO;

public SpelLoadBalancersMigration(PipelineDAO pipelineDAO) {
this.pipelineDAO = pipelineDAO;
}

@Override
public boolean isValid() {
return true;
return clock.instant().toEpochMilli() < VALID_UNTIL.getTime();
}

public void run() {
Expand Down