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

Add the --rebuild option to the k8s-env command #3480

Merged
merged 12 commits into from
Dec 4, 2023
Merged

Conversation

busma13
Copy link
Contributor

@busma13 busma13 commented Dec 1, 2023

This PR includes the following changes:

  • Add the --rebuild option to ts-scripts k8s-env
  • Use yarn k8s:rebuild script from root of teraslice
  • This allows you to build a new Teraslice docker image and restart Teraslice in your k8s cluster. Previously you had to delete the cluster and re-run yarn k8s. Now services and the cluster do not need to be rebuilt.
  • Changes to how teraslice and elasticsearch ports are determined.
  • Elasticserach store data is preserved between rebuilds. We may need to add the option to reset the ES store.

@busma13 busma13 requested a review from godber December 1, 2023 22:15
@godber
Copy link
Member

godber commented Dec 1, 2023

Elasticserach store data is preserved between rebuilds. We may need to add the option to reset the ES store.

Yeah, there will be cases for both.

Copy link
Member

@godber godber left a comment

Choose a reason for hiding this comment

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

Approved, most of my comments are either general advice or stream of conscious comments.

@@ -22,12 +22,15 @@ const { cleanupIndex } = ElasticsearchTestHelpers;

const generateOnly = GENERATE_ONLY ? parseInt(GENERATE_ONLY, 10) : null;

const TERASLICE_PORT = 45678;
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 sure if this is accessible in this scope, but here's another place this port is defined:

In the scripts package, so maybe it does't make sense to have this dependency, or maybe it should go the other way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • The teraslice port was a magic number in e2e/test/teraslice-harness.js and e2e/test/global.setup.js, so I just made it a constant so things were more explicit. I should be able to pass the value from one to the other.
  • In my first attempt to declare the teraslice port, I placed it in helpers/scripts.ts, then modified it if we were in k8s-env mode, not testing mode. I then removed the declaration in favor of passing the port in from either the global.setup.js constant or the k8s-env command options.
  • Ports are also hard coded into some yaml files. It would be nice to set a variable once and pass it everywhere needed!

@@ -16,6 +16,8 @@ const {
CONFIG_PATH, ASSETS_PATH, TEST_PLATFORM
} = require('./config');

const TERASLICE_PORT = 45678;
Copy link
Member

Choose a reason for hiding this comment

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

You have this variable set elsewhere too.

Copy link
Member

Choose a reason for hiding this comment

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

Is is a problem if it gets updated in one place but not the other? If so, you have a problem.

}

async deleteTerasliceNamespace() {
async confirmDeletion(terasliceNamespace: string, coreV1Api: k8sClient.CoreV1Api) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a pretty vague function name, if it means confirmNamespaceDeletetion you might name it that.

@@ -21,7 +21,6 @@ import { getE2eK8sDir } from '../helpers/packages';
import { yamlDeploymentResource, yamlServiceResource } from './k8s-env/interfaces';

const logger = debugLogger('ts-scripts:cmd');
let TS_PORT = '45678';
Copy link
Member

Choose a reason for hiding this comment

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

Now I'm confused about this TS_PORT thing.

@godber godber merged commit 3d4430c into master Dec 4, 2023
39 checks passed
@godber godber deleted the k8s-env-rebuild-option branch December 4, 2023 20:59
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