-
Notifications
You must be signed in to change notification settings - Fork 72
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
Deployer missing files #1599
Deployer missing files #1599
Conversation
const currentOra = getSpinner(); | ||
if (currentOra.isSpinning) { | ||
currentOra.clear(); | ||
currentOra.frame(); | ||
} | ||
oraInstance().warn(text); | ||
} | ||
|
||
export function info(text: string) { | ||
const currentOra = getSpinner(); | ||
if (currentOra.isSpinning) { | ||
currentOra.clear(); | ||
currentOra.frame(); | ||
} | ||
oraInstance().info(text); | ||
} | ||
|
||
export function debug(text: string) { | ||
if (debugModeFlag) oraInstance().info(text); | ||
if (debugModeFlag) info(text); |
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.
These changes are to allow printing a line to the console while the ora
spinner is still spinning. WIthout these the spinner will overwrite any logs.
const latestDepolymentFileNames = Object.keys((stageDirectory.children[latestDeployment] as Directory).children); | ||
|
||
const requiredFileNames = ['config.json', 'secrets.env', 'default.tfstate']; | ||
const missingRequiredFiles = requiredFileNames.filter( | ||
(requiredFileName) => !latestDepolymentFileNames.includes(requiredFileName) | ||
); | ||
if (missingRequiredFiles.length) { | ||
logger.warn( | ||
`Airnode '${airnodeAddress}' with stage '${stage}' is missing files: ${missingRequiredFiles.join( | ||
', ' | ||
)}. Deployer commands may fail and manual removal may be necessary.` | ||
); | ||
} | ||
|
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 will detect any of the three missing files, but note that if the missing file is either secrets.env
or config.json
, it is not possible to derive the deploymentId
so the commands will fail with an error:
No deployment with ID 'aws...' found
So would it make sense to just throw an error directly if either of these files are missing (but presumably the deployment is found). But this could exit the loop too early if there are multiple deployments in the bucket 🤔
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.
Nice start but I think we need to handle this more thoroughly.
There are currently 4 deployer commands (and more will come in the future):
- deploy - Deploy is not using
fetchDeployments
so it needs to be handled separately there. For deploy to work (assuming previous deployments) we need all three files (config.json
,secrets.env
anddefault.tfstate
) to be present. The deployment won't work without them so I would just fail with an error message if one of them is missing. - remove - Remove is using
fetchDeployments
because it needs to find the deployment based on the deployment ID. Again, we need all three files for deployment removal to work. And again, we can just fail if some of them are missing. - list - List doesn't really care about the
default.tfstate
file, just theconfig.json
andsecrets.env
. Here, it makes to skip the invalid ones (with a message) but still print those that are fine. - info - Again, doesn't really care about the
default.tfstate
. It's concerned only with one deployment and its historical records. But without that deployment having all the files we can't really say whether it's there just broken or is not there at all 🤔
My thinking here is that before running any actual functionality of these commands, we should do a sanity check on all the deployments in the bucket and print warnings accordingly. And then follow with whatever the command is supposed to do and react accordingly depending on the command (e.g. fail). WDYT?
Another thing is that maybe we should rethink making deployment ID from data that is in the files. Or rather moving that data into the directory structure of the bucket. That way we would be able to distinguish between a deployment missing and being broken. But that's a different topic.
Thanks for reviewing. I am currently finishing up 2 tasks for operations and will get back to this one after. |
This case has been handled now and is ok since we're only looking for a single deployment.
This case is proving to be harder to implement since we can't be sure of the
Yes definitely this seems to be required to handle either missing or broken deployment checks properly. |
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'm fine with this solution for now. Later we should rethink the deployment ID generation though. But I don't want to block the 0.10 release even more and it's good enough.
Just some minor issues.
Thanks! The final changes should be covered now. |
Closes #1449