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

feat: deployment related fixes #647

Merged
merged 7 commits into from
Jan 28, 2025
Merged

feat: deployment related fixes #647

merged 7 commits into from
Jan 28, 2025

Conversation

Mirko-von-Leipzig
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig commented Jan 27, 2025

This PR has a variety of fixes for the faucet and deployment scripts.

Faucet fixes

Changes actually affecting other operators.

  • Faucet webpage was broken because I removed /*path which removed serving of index.css and index.js. I re-added them as constant paths instead. Note that I also tried supporting /index.html which is a common entry-point/alias for / but this didn't work - the html script attempts to fetch /index.html/index.js.
  • Faucet webpage endpoint is now public: localhost -> 0.0.0.0 by default. This is so deployments are automatically public instead of private.

Deployment changes

Changes to our deployment workflow scripts.

  • Remove --features testing vestiges (fix).
  • Limit workflow concurrency to 1-per-network.
  • Fix naming of deb packages when deploying a branch. The naming scheme used the branch name - however this causes issues when the branch name contains a / e.g. mirko/fixup_deploy. Instead we now assign a unique name per workflow run. This name is only used to transfer between github -> S3 -> deploy instance.

Good news

With these changes the deployment did #just-work alongside the DNS related devops changes.

faucet.devnet.miden.io works as expected.

rpc.devnet.miden.io works with the client once the TLS issue (0xPolygonMiden/miden-client#696) is addressed.

Release versions

How do we want to handle versioning given that v0.7.0 is already on crates.io? At minimum we need the faucet route fixes; which we could separate out and release as v0.7.1. Though ideally for deployment we want to also just deploy the release version, so having both would be nice.

Ideally we also need to make the github release so we can tag v0.7.0 for crates.io to link against properly.

Future facing notes

toml configuration

Using toml as the sole configuration format is pretty painful for scripting. As an example, ideally we would keep the defaults of all endpoints as localhost as this is safer.

We also want to use internal DNS values without making them the default e.g. store.devnet.miden.io but we cannot do that without additional scripting.

How does one edit toml files using a script? I guess one could get an additional binary that performs toml edit'ing. However this now becomes an additional tool that needs to be deployed on servers.

You might consider using sed however that becomes tricky because toml can have identical lines under different sections i.e. you cannot simply replace endpoint = ... because there are multiple such lines -- so you need some section awareness.

It basically makes it painful to change defaults in a non-manual manner.

Personally I would advocate that we should default to cli configuration, with env variable support (trivial with clap). We could even keep the toml files; though I would argue that they are obsolete because one just as easily create a .env file with environment variables.

Endpoints

We use the Endpoint type which splits the concern of host, port and protocol. I think we should remove it entirely and delegate that responsibility entirely to the http client/server crates we use. I think all of them take a &str or similar argument in any case.

They're a leaky abstraction imo; just take a string and let the experts deal with it.

Static files

Low priority, but we could consider a better html solution. Especially if we want to add more in the future as a configuration/admin dashboard.

There are some interesting developments with htmx and crates like maud and askama for embedding html.

Status

faucet.devnet.miden.io is down after re-deploying. Waiting on support.

@Mirko-von-Leipzig Mirko-von-Leipzig force-pushed the mirko/fixup_deploy branch 2 times, most recently from bdd5c7f to d637d11 Compare January 27, 2025 09:26
@Mirko-von-Leipzig Mirko-von-Leipzig marked this pull request as ready for review January 27, 2025 17:09
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! For deployment scripts, I did a very light review.

How do we want to handle versioning given that v0.7.0 is already on crates.io? At minimum we need the faucet route fixes; which we could separate out and release as v0.7.1. Though ideally for deployment we want to also just deploy the release version, so having both would be nice.

I think there are options:

  1. Increment the faucet version to v0.7.1 and publish it on crates.io. Don't create a tag.
  2. Same as option 1, but create a v0.7.1 tag. This will mean that other creates that are at v0.7.0 will be tagged as v0.7.1.
  3. Increment all crate versions to v0.7.1, publish them to crates.io and tag a new release on Github.

I'm on the fence between options 2 and 3. Option 3 is a bit more work, but also probably a more correct way to approach this.

Personally I would advocate that we should default to cli configuration, with env variable support (trivial with clap). We could even keep the toml files; though I would argue that they are obsolete because one just as easily create a .env file with environment variables.

I don't mind switching to .env files. Let's create an issue for this and unless there are objects, we can migrate from TOML to .env.

We use the Endpoint type which splits the concern of host, port and protocol. I think we should remove it entirely and delegate that responsibility entirely to the http client/server crates we use. I think all of them take a &str or similar argument in any case.

I'm fine with this as well. Let's create an issue.

Static files

Low priority, but we could consider a better html solution. Especially if we want to add more in the future as a configuration/admin dashboard.

There are some interesting developments with htmx and crates like maud and askama for embedding html.

I don't know much about these, but let's also create an issue. We can come back to this in some future milestone.

@bobbinth bobbinth changed the base branch from next to main January 28, 2025 04:42
@bobbinth bobbinth changed the base branch from main to next January 28, 2025 04:42
@bobbinth
Copy link
Contributor

Also, could you rebase this from main. Since this will go into v0.7.x release, we can merge it into main and then merge back into next since next is now on v0.8.0 release.

@Mirko-von-Leipzig Mirko-von-Leipzig changed the base branch from next to main January 28, 2025 08:11
@Mirko-von-Leipzig
Copy link
Contributor Author

How do we want to handle versioning given that v0.7.0 is already on crates.io? At minimum we need the faucet route fixes; which we could separate out and release as v0.7.1. Though ideally for deployment we want to also just deploy the release version, so having both would be nice.

I think there are options:

  1. Increment the faucet version to v0.7.1 and publish it on crates.io. Don't create a tag.
  2. Same as option 1, but create a v0.7.1 tag. This will mean that other creates that are at v0.7.0 will be tagged as v0.7.1.
  3. Increment all crate versions to v0.7.1, publish them to crates.io and tag a new release on Github.

I'm on the fence between options 2 and 3. Option 3 is a bit more work, but also probably a more correct way to approach this.

Ah right the issue is that we can't only tag the faucet version. There is probably some solution to this; but I think we shouldn't bother. I vote for option 3, that way we also have a correct v0.7.0 and v0.7.1 tag.

... issues

Issues create as #652 (html) #651 (endpoint) and discussion #650 (config).

@bobbinth bobbinth merged commit c11a67e into main Jan 28, 2025
10 checks passed
@bobbinth bobbinth deleted the mirko/fixup_deploy branch January 28, 2025 09:02
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