Skip to content

Conversation

@MicaiahReid
Copy link
Contributor

@MicaiahReid MicaiahReid commented Jul 26, 2023

This PR:

  • adds the HEAD /api/v1/network/{network} route
    • this route returns OK 200 if any devnet assets exist for the provided network. If this endpoint is responding with 200, any attempts to POST /api/v1/networks with this same network key will fail.
    • this route returns 404 if no devnet assets exist for the provided network. This indicates that a POST /api/v1/networks should be successful for that network
  • POST /api/v1/networks checks if any assets exist for the given network id before creating a devnet. If they do, no new assets are created
  • DELETE /api/v1/network/{network} checks if any assets exist before deleting devnet assets for the given network id. If any exist, devnet assets are deleted. If not, the function returns without attempting any deletions.
  • GET /api/v1/network/{network} checks to ensure all devnet assets are created before getting devnet info
  • GET/POST /api/v1/network/{network}/* checks to ensure all devnet assets are created before proxying requests

Fixes #36, fixes #39, fixes #5

@MicaiahReid MicaiahReid changed the title Check assets feat: add HEAD /api/v1/network/{network} route Jul 26, 2023
@leahjlou
Copy link
Collaborator

All of these changes sound great 👍

Question on this:

GET /api/v1/network/{network} checks to ensure all devnet assets are created before getting devnet info

Will this return some kind of Pending state before all the assets have been created?

.try_log(|logger: &hiro_system_kit::Logger| slog::info!(logger, "{}", msg));
Err(DevNetError {
message: msg,
code: 404,
Copy link
Collaborator

@leahjlou leahjlou Jul 26, 2023

Choose a reason for hiding this comment

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

To answer my own question - it looks like no, this would return a 404, not something indicating a "pending" state. To determine if a devnet is in process of starting up, would the right check be if this GET returns a 404 but the HEAD returns a 200? That implies that some, but not all, assets exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that's a good question. Having to check both endpoints would be tedious.

Whenever you create a devnet, all assets should nearly immediately be created*. If you immediately make this GET request, it should not return a 404 response, and the status should show as "Pending" for a pod that is starting up. Deletion, however, takes some time, so if you hit the DELETE endpoint, then query this endpoint, you should get 404 because some items are deleted and others aren't.

If you find that you're getting 404s a lot when you don't think you should, let me know. Maybe we can find a better fix than this. I definitely don't want you to have to see if GET returns 404 and HEAD returns 200 to know if a devnet is starting.

*There is currently an exception on all assets being created very quickly. When you receive a response from creating a devnet all assets should exist in kubernetes. However, if you query the GET endpoint before the creation request is made, you could get a 404 on the GET request. I currently have a 5 second delay in the middle of devnet creation, which I will hopefully be able to remove soon. This will make creating a devnet much faster.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes sense, and should work just fine for the app. If I'm seeing more 404s than expected I'll definitely let you know.

@MicaiahReid MicaiahReid requested a review from leahjlou July 26, 2023 21:40
@MicaiahReid MicaiahReid merged commit 1bf329f into main Aug 1, 2023
github-actions bot pushed a commit that referenced this pull request Nov 16, 2023
## 1.0.0 (2023-11-16)

### Features

* add `HEAD /api/v1/network/{network}` route ([#41](#41)) ([1bf329f](1bf329f))
* add logging and network info route ([#20](#20)) ([2af0bab](2af0bab)), closes [#21](#21)
* proxy http requests to downstream pods ([#11](#11)) ([6ecdf0f](6ecdf0f))
* release develop ([#84](#84)) ([89a1a1b](89a1a1b))

### Bug Fixes

* add access_control_allow_credentials header ([a482a93](a482a93))
* add cors settings; refactor http responses ([#42](#42)) ([c46db4c](c46db4c)), closes [#21](#21)
* assert more general error msg ([#48](#48)) ([926e3a0](926e3a0))
* create namespace in deploy api script ([f5ff5e0](f5ff5e0))
@github-actions
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

3 participants