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

Kraken: Add Manifest V2 with multiple manifest support #2607

Merged

Conversation

JoaoMario109
Copy link
Collaborator

Depends on #2604

@JoaoMario109 JoaoMario109 force-pushed the add-manifest-v2-module branch 24 times, most recently from d7458a2 to 642eadd Compare May 21, 2024 23:00
@JoaoMario109 JoaoMario109 force-pushed the add-manifest-v2-module branch 2 times, most recently from 1e90798 to 4aee47f Compare May 23, 2024 19:31
@JoaoMario109 JoaoMario109 marked this pull request as ready for review May 23, 2024 21:45
Copy link
Member

@Williangalvani Williangalvani left a comment

Choose a reason for hiding this comment

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

This is looking very good!

I have 3 comments though:

  • Can we show on the list if a manifest is disabled? currently we need to click the edit button.
  • Can we decrease the timeouts? 20s is a LONG time for waiting before giving the user some feedback, specially on post/delete.
  • we should add some url validation in the form, just in case. (I added gibberish and it gave me an internal error)

@JoaoMario109 JoaoMario109 force-pushed the add-manifest-v2-module branch 3 times, most recently from 5b14f4e to 4b16d3a Compare May 27, 2024 15:36
Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

ops, wrong button

@patrickelectric patrickelectric enabled auto-merge (rebase) May 29, 2024 11:54
@patrickelectric
Copy link
Member

It'll be merged once the change request from @Williangalvani get solved. (Needs approval)

@patrickelectric patrickelectric merged commit 4fbbdee into bluerobotics:master May 29, 2024
6 checks passed
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.

None yet

3 participants