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

Improve Docker image interfaces #1189

Closed
amarthadan opened this issue Jun 13, 2022 · 11 comments · Fixed by #1322
Closed

Improve Docker image interfaces #1189

amarthadan opened this issue Jun 13, 2022 · 11 comments · Fixed by #1322
Assignees
Milestone

Comments

@amarthadan
Copy link
Contributor

Slack discussion
Currently, the Docker image interface is extremely demanding UX-wise. This should be much easier to use and not require you to know anything about the inner workings of the image. For example, -r can be an optional parameter that expects a particular directory structure (as in pre-alpha expected config.json and security.json to be in cd).

For reference API providers have no idea about what they are doing with the commands and even we, the internal users can’t easily tell if there is an error in a command or not.

This needs proper research and discussion.

@amarthadan amarthadan added this to the 0.8.0 milestone Jun 13, 2022
@bbenligiray
Copy link
Member

@amarthadan Can you tell a bit about the reasoning behind the config/ and output/ directories? Is it because we don't want to expose the file that keeps the cloud provider credentials to the container?

@amarthadan
Copy link
Contributor Author

@amarthadan Can you tell a bit about the reasoning behind the config/ and output/ directories? Is it because we don't want to expose the file that keeps the cloud provider credentials to the container?

No, I think we will do that in #1205 anyway.
You need to be able to pass the configuration files to the container and retrieve the receipt.json from the container in some way. I've just decided to separate the input (config.json) and output (receipt.json) into different directories/volumes. But it can be all in one directory. Or do you have something else in mind?

@bbenligiray
Copy link
Member

I mean everything used to be in a single directory and you implemented the two directories. I think if there is a good reason to have multiple directories we should always do that or if not we should always use a flat directory. Having two options will complicate things.

@amarthadan
Copy link
Contributor Author

You have always multiple options, it's up to you how you map the directories into a container. But yes, we can encourage (and write docs and examples that way) one.

So, would you like to keep everything (config.json, secrets.env, aws.env, gcp.json, and receipt.json) in one config directory? It is possible to do it that way. I personally, as a user, would prefer the tool not to output anything among configuration files (receipt.json) but that might be just my preference.

Another issue I think you encountered and didn't like is the -r option to remove Airnode deployment via receipt.json. We can't really set a default for that option (like we do for config.json and secrets.env) because you don't need to provide a receipt to remove an Airnode, you can also provide other parameters, describing the deployment (e.g. stage, short address, etc.). What we can do, is divide the remove command into two, one removing via receipt and one using descriptive parameters.

@bbenligiray
Copy link
Member

So, would you like to keep everything in one config directory?

I think this is much easier to communicate, so makes things easy for the new user. We already display a message about outputting the receipt.json, which is something.

What we can do, is divide the remove command into two

I don't see any downsides to this, feels like the right way to go

@dcroote
Copy link
Contributor

dcroote commented Jun 29, 2022

Summarizing, this entails:

  • Reconfiguring the directory structure: remove the output/ directory and instead output receipt.json to the config/ directory

  • Dividing the deployer remove command in two: one version, remove-with-receipt that assumes (but accepts as an option) the receipt.json is in config/ and the other remove-with-deployment-details that accepts other parameters that allow for airnode removal

@dcroote
Copy link
Contributor

dcroote commented Jun 29, 2022

@amarthadan - do I have the above summary correct? Anything missing?

@amarthadan
Copy link
Contributor Author

Yes, I think that's correct 🙂 Do you want me to break it into two smaller issues, or are you fine with this one?

@dcroote
Copy link
Contributor

dcroote commented Jun 29, 2022

I think this one should work fine.

@martinkolenic
Copy link
Contributor

Hi @dcroote, I've run test deployments of the basic coingecko example both on AWS and GCP. I basically followed the standard example scenario but instead of running the remove-airnode script, I ran the docker commands for airnode-deployer.

  1. I ran remove-with-receipt, which completed fine.
  2. Then, I redeployed and remove-with-deployment-credentials with receipt.json moved out of the config directory and typing in the data from the file as command parameters. This also worked.
  3. Finally, just for good measure, I redeployed again, left the receipt.json file where it was but ran remove-with-deployment-details again, to check nothing breaks when the file stays put. This worked as well, no hiccups.

Moving this to Done :)

@dcroote
Copy link
Contributor

dcroote commented Jul 14, 2022

Fantastic, thanks for the thorough testing @kolenic-martin

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 a pull request may close this issue.

4 participants