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

test: Added upgrade tests #5072

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

tmihalac
Copy link
Contributor

@tmihalac tmihalac commented Feb 19, 2025

Added feast go operator upgrade tests

What this PR does / why we need it:

Tests backward compatibility of the CR

There are 2 new test suites with new make targets:
test-previous-version - Deploys the previous version operator and feast server, applies the previous version CRs and runs the e2e test
test-upgrade - Deploys the previous version operator and feast server, applies the current CRs and runs the e2e test

Those tests are run in the integration test github action right after the make test-e2e target

@tmihalac
Copy link
Contributor Author

Please have a look I've split the pervious version test and the upgrade version test into 2 test suites, so I might call them from the OpenshiftCI @tchughesiv @lokeshrangineni @redhatHameed

@tmihalac tmihalac changed the title Added upgrade tests test: Added upgrade tests Feb 19, 2025
@tmihalac tmihalac force-pushed the upgrade-tests branch 4 times, most recently from 8daf89c to 298306c Compare February 19, 2025 14:23
@tmihalac tmihalac force-pushed the upgrade-tests branch 15 times, most recently from fc73482 to b23a78a Compare February 19, 2025 19:14
@tmihalac tmihalac marked this pull request as ready for review February 20, 2025 15:03
@tmihalac tmihalac requested a review from a team as a code owner February 20, 2025 15:03
feastResourceName + "-online",
feastResourceName + "-offline",
feastResourceName + "-ui",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

just want make sure this is correct, we have online store/server default now. so need registry here instead of online. WDYT ?
cc @tchughesiv

Copy link
Contributor

Choose a reason for hiding this comment

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

or, just add feastResourceName + "-registry",

Copy link
Contributor Author

@tmihalac tmihalac Feb 20, 2025

Choose a reason for hiding this comment

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

just want make sure this is correct, we have online store/server default now. so need registry here instead of online. WDYT ? cc @tchughesiv

@redhatHameed @tchughesiv I used the old e2e tests without changing them and in them there is special care of registry, i didn't want to mix updating/fixing e2e tests following latests changes with doing an upgrade test, I think it should be a different task

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe try to change feastResourceName + "-online" -> feastResourceName + "-registry" and see where it breaks ? if this required more works then we can open a separate PR for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe try to change feastResourceName + "-online" -> feastResourceName + "-registry" and see where it breaks ? if this required more works then we can open a separate PR for this.

It causes the tests to fail, I think it should be in a different task

Copy link
Contributor

Choose a reason for hiding this comment

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

i think we need a different method maybe? one that checks for remote registry and one that checks for local registry service. when remote registry is configured, a svc won't exist for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we need a different method maybe? one that checks for remote registry and one that checks for local registry service. when remote registry is configured, a svc won't exist for it

Do you think it should be fixed as part of the upgrade tests ? IMO it should be a separate task - "revise existing e2e test and enhance them"

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened an new issues to track this #5091

@redhatHameed
Copy link
Contributor

redhatHameed commented Feb 20, 2025

@tmihalac few minor questions

  1. Are the upgrade tests set to run during the Feast release process, or every pull request?
  2. What will happened if test fails will this block releases ?

@tmihalac
Copy link
Contributor Author

tmihalac commented Feb 20, 2025

@tmihalac few minor questions

  1. Are the upgrade tests set to run during the Feast release process, or every pull request?
  2. What will happened if test fails will this block releases ?

@redhatHameed I've added them to the e2e github action which runs on:

  push:
    branches:
      - main
  pull_request:
    types:
      - opened
      - synchronize
      - labeled```

Signed-off-by: Theodor Mihalache <[email protected]>
@franciscojavierarceo franciscojavierarceo merged commit d1bf58f into feast-dev:master Feb 25, 2025
24 checks passed
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.

5 participants