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(apps): Create application via apps deploy #272

Merged
merged 8 commits into from
Mar 8, 2022

Conversation

raulb
Copy link
Member

@raulb raulb commented Mar 8, 2022

Description of change

Fixes https://github.com/meroxa/turbine-project/issues/71

  • Adds validation to golang before carrying on with other steps (consistent with the API language accepted languages)
    - [x] Adds list and describe as commands to execute

Type of change

  • New feature
  • Bug fix
  • Refactor
  • Documentation

How was this tested?

  • Unit Tests
  • Tested in staging

Demo

See slack

@raulb raulb self-assigned this Mar 8, 2022
@raulb raulb marked this pull request as ready for review March 8, 2022 12:57
@raulb raulb requested a review from janelletavares March 8, 2022 12:57
}

// Built was successful, we're ready to create the application
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be first. I think the eventual sequence is going to be 1) make pipeline 2) make app 3) make all entities within the pipeline that also belong to the app. If there's a name conflict or any reason that we can't make the app, then I don't think we should have created app entities at that point. Did i misunderstand?

Suggested change
// Built was successful, we're ready to create the application
// Build was successful, we're ready to create the application

Copy link
Member Author

Choose a reason for hiding this comment

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

@janelletavares we'll only create the resources (app, pipeline, etc...) if uploading the image (build) was successful. I'm actually making some changes on this branch that are related to those steps as well. Trying to not increase much scope on this PR, but still deliver something that actually applies to the direction we want to take. I'll keep you posted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @janelletavares that the user experience could be improved by having the app name validation (which is almost immediate) fail before the source upload and function image build have finished completely (which would take some time)

I can imagine though that the creation of connectors could be postponed to a later time point than the function build. The same would apply to creating the function objects which depend on the function image build to finish beforehand

I don't want to block this PR from moving forward, just mentioning this already, in case there's interest on iterating on it in the future

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with @janelletavares that the user experience could be improved by having the app name validation (which is almost immediate) fail before the source upload and function image build have finished completely (which would take some time)

We documented we'd be creating the app only if build was successful (we'll be doing a long polling and blocking the next steps until building is successful) so we don't need to worry about orphan apps and then let users do another re-deploy in an attempt to build again (we don't have app updates available).

I understand what you two are mentioning regarding the UX, and I'll give this another thought however what I'm trying to accomplish here is to leave the step that less likely will fail to the end. In other words, trying to build several times and leave different building steps hanging is completely fine, however having an application created (and not deleted) if something after failed not so much (we're creating turbine apps after all).

Happy to discuss different corner cases and possible scenarios (with its UX improvements).

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting about doing the steps most likely to fail first. thanks for the clarification.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good and thank you for the outline!

@raulb raulb force-pushed the raul/create-apps-on-deploy branch from 010ea7a to f5a06bc Compare March 8, 2022 20:49
raulb and others added 2 commits March 8, 2022 22:57
@raulb raulb merged commit 0c72c97 into master Mar 8, 2022
@raulb raulb deleted the raul/create-apps-on-deploy branch March 8, 2022 22:22
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.

3 participants