-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
.werft/build: Refactor installer steps into dedicated class #8884
Conversation
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
/werft run with-vm=true |
Tested on both core-dev and Harvester, so turning it into ready for review 🙂 |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome Arthur. I think this could be improved further, I have left some comments in the review. I'm happy to talk through this more if that's helpful
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
Thanks for all the Typescript tips Mads, the CI is still re-running but I believe I was able to do the refactoring using class and private methods 🙂 Learned a lot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments that you can follow up on in a later PR
import { Werft } from "../../../util/werft"; | ||
import { getNodePoolIndex } from "../deploy-to-preview-environment"; | ||
|
||
const blockNewUserConfigPath = './blockNewUsers'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We don't (as far as I know) have a convention to use upper or lower case for constants like these, but it would be nice to be consistent within the file. So either make these uppercase of make the uppercase ones lowercase
workspaceFeatureFlags: string[] | ||
gitpodDaemonsetPorts: GitpodDaemonsetPorts | ||
|
||
constructor(werft: Werft, configPath: string, version: string, proxySecretName: string, domain: string, previewName: string, imagePullSecretName: string, deploymentNamespace: string, analytics: Analytics, withEELicense: boolean, withVM: boolean, workspaceFeatureFlags: string[], gitpodDaemonsetPorts: GitpodDaemonsetPorts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you have a method/function that takes a lot of arguments I usually prefer to use an "options" argument instead (just as you have previously done here.) I has a few benefits
- It becomes easier to read what the arguments are as they're named when you invoke the function. This is nice when you're reading code in a PR and not in your editor.
- It's easier to add new arguments as you don't have to worry about the order.
So in this case it would look like this
export type InstallerOptions = {
werft: Werft
configPath: string
// all the rest
}
export class Installer {
// All your private members
constructor(options: InstallerOptions) {
this.werft = options.werft
// all the rest
}
}
And then when you invoke it
new Installer({
werft: werft,
configPath: configPath,
/// and so on
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice, I really wasn't enjoying that method signature 😅
I'll change that real quick
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
Signed-off-by: ArthurSens <arthursens2005@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, looks great 🛹
Description
This PR tries to reduce amount of code under
.werft/jobs/build/deploy-to-preview-environments.ts
by moving all code related to installer to a dedicatedInstaller
interface.That way, we can make the deploy phase as simple as:
I'm opening it as a draft just to collect early feedback on the approach. @gitpod-io/platform do you agree with the approach here?
How to test
Release Notes