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

New application entity (v0.3.0) #447

Merged
merged 8 commits into from
Apr 28, 2021

Conversation

dhiguero
Copy link
Contributor

@dhiguero dhiguero commented Apr 12, 2021

What does this PR do?

  • Updates the specification of the Application entity according to the v0.3.0 spec proposal.

Where should the reviewer start?

  • Review 7.application.md for most changes.

What is missing?

Any background context you want to provide?

What are the associated tickets?

Questions

  • Section Component instances and revisions of component has been updated with components, but this is closely related to how the runtime is expected to operate. Is this still the intended behavior? I think it is necessary to create a component instance since we are referencing components by type and assigning them a new name, which I suppose will be used to name the component instance.

@ghost
Copy link

ghost commented Apr 12, 2021

CLA assistant check
All CLA requirements met.

@ghost
Copy link

ghost commented Apr 12, 2021

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ dhiguero sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@dhiguero dhiguero changed the title New application entity New application entity (v0.3.0) Apr 12, 2021
Copy link
Member

@hongchaodeng hongchaodeng left a comment

Choose a reason for hiding this comment

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

lgtm after nit

7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated
In practice, the available workload types are registered by infrastructure operators/platform builders to the platform following OAM specification. They are explained in detail in this chapter: [workload definition](4.workload_types.md).
A component may define a set of parameters that can be modified for each particular instance.

upon instanciation.
Copy link
Member

Choose a reason for hiding this comment

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

What's this line?

7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated
traits:
- name: scaler
- name: ingress # ingress trait providing a public endpoint for the publicweb component of the application.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe leave a comment here indicating that the name of the trait will be the same as the component?

dhiguero and others added 2 commits April 15, 2021 20:51
Co-authored-by: Hongchao Deng <hongchaodeng1@gmail.com>
Co-authored-by: Jianbo Sun <wonderflow.sun@gmail.com>
Co-authored-by: Lei Zhang (Harry) <resouer@gmail.com>
@dhiguero
Copy link
Contributor Author

Hi! I have been testing deploying an Application in Kubevela following the proposed changes and it seems that there is a mismatch related to how scopes are specified. In the previous version, we used a scopeRef as:

      scopes:
        - scopeRef:
            apiVersion: core.oam.dev/v1alpha2
            kind: HealthScope
            name: app-health # An application level health scope including both components.

However it seems that the new runtime expects a map as:

      scopes:
        "healthscopes.core.oam.dev": "app-health"

@resouer, @hongchaodeng Could you please check which one is the expected one? I will update the example accordingly before merging the PR.

@wonderflow
Copy link
Member

In application, I hope we can use this way as it's more convenient:

      scopes:
        "healthscopes.core.oam.dev": "app-health"

The healthscopes.core.oam.dev will refer to a ScopeDefinition which will refer to the information:

            apiVersion: core.oam.dev/v1alpha2
            kind: HealthScope

@resouer
Copy link
Member

resouer commented Apr 24, 2021

+1 to @wonderflow 's approach. Note that this also implies the ScopeDefinition is named as healthscopes.core.oam.dev and references the real resource schema (HealthScope GVK), similar to workload definition.

@resouer resouer added this to the v0.3.0 milestone Apr 25, 2021
@dhiguero
Copy link
Contributor Author

@resouer , @wonderflow I have uploaded the changes related to the scope.

7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
7.application.md Outdated Show resolved Hide resolved
@dhiguero
Copy link
Contributor Author

New revision available. Thanks @kminder for the awesome review.

Copy link

@kminder kminder left a comment

Choose a reason for hiding this comment

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

Approved

Copy link
Member

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

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

Thanks, great job!

@resouer resouer merged commit 9ce53b6 into oam-dev:master Apr 28, 2021
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.

5 participants