-
Notifications
You must be signed in to change notification settings - Fork 114
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
DevOps - Update Colossus deployment for Giza #2829
Conversation
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.
Deployment works well. Handling of keyFile/passphrase and accountUri configs needs improvement.
}, | ||
}) | ||
const remoteKeyFilePath = '/joystream/key-file.json' | ||
additionalParams = ['--key-file', remoteKeyFilePath] |
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.
keyfile is required above but additionalParams are not used, since we are passing ACCOUNT_URI.
So we need better handling I think here of keyFile, passphrase, and accountUri configs, especially if both are set, which one takes precedence (based on storage-node implementation of course)
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.
So according to the method getAccount
in storage-node-v2/src/command-base/ApiCommandBase.ts
if accountURI
is specified it takes precedence and overrides password
and keyFile
flags.
Also either Keyfile
or accountURI
must be set otherwise we throw an error.
Will update this in the code.
811ff24
to
e395c5f
Compare
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.
Minor issue with the port configuration otherwise looks good.
'server', | ||
'--worker', | ||
workerId, | ||
'--port=3333', |
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.
Shouldn't this be the the pulumi configuration parameter 'colossusPort' instead of hard-coded 3333?
value: `${colossusPort}`, | ||
}, | ||
{ | ||
name: 'QUERY_NODE_HOST', |
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.
this has now changed to QUERY_NODE_ENDPOINT and is a full Url not just the host-name
queryNodeHost
,workerId
,accountURI
,colossusImage
configMap
topulumi-common
Manual testing
Running stack with these configurations
![Screenshot 2021-12-23 at 6 26 43 PM](https://user-images.githubusercontent.com/7795956/147244037-031cf793-7104-4b36-8bd6-fc76a74e4751.png)
The node runs without any issues
![Screenshot 2021-12-23 at 6 26 38 PM](https://user-images.githubusercontent.com/7795956/147244052-65a36c09-902b-411f-9a68-ebdd21f78835.png)