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

Add docs for extraDevfileEntries.yaml #445

Merged
merged 1 commit into from May 10, 2021

Conversation

GeekArthur
Copy link
Contributor

Signed-off-by: jingfu wang jingfwan@redhat.com

What does this PR do?

This PR adds docs for extraDevfileEntries.yaml under registry structure

What issues does this PR fix or reference?

#279
#403
#409

Is your PR tested? Consider putting some instruction how to test your changes

This is a docs PR.

Signed-off-by: jingfu wang <jingfwan@redhat.com>
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: GeekArthur

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@maysunfaisal maysunfaisal left a comment

Choose a reason for hiding this comment

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

should this doc talk about what we discussed with Elson ie icon can be URI or base64 🤔

@GeekArthur
Copy link
Contributor Author

GeekArthur commented Apr 29, 2021

should this doc talk about what we discussed with Elson ie icon can be URI or base64 🤔

I think this again, the base64 we discussed before is in index.json, but this docs is for extraDevfileEntries.yaml, extraDevfileEntries.yaml doesn't include any base64. The way we handle the base64 is we parse the icon fields in the existing devfiles and extraDevfileEntries.yaml then generate the base64 in the index.json.

Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

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

While this is a good spot to have this information from a registry design perspective, when we start working on user-facing docs for the devfile registry, we need to make sure this information is captured there too.

@GeekArthur
Copy link
Contributor Author

While this is a good spot to have this information from a registry design perspective, when we start working on user-facing docs for the devfile registry, we need to make sure this information is captured there too.

Exactly, I think the user-facing docs is tracked by this issue: #191 and finally it will be published to our official website (eg. subdomain for devfile registry docs).

@GeekArthur GeekArthur requested a review from elsony May 4, 2021 14:37
@GeekArthur
Copy link
Contributor Author

@johnmcollier @maysunfaisal Can you please review this PR again?

Comment on lines +54 to +56
git:
remotes:
origin: https://github.com/redhat-developer/devfile-sample
Copy link
Member

Choose a reason for hiding this comment

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

Loooks good to me, except maybe one ques that pops into my head

what is the expected outcome(index) if git has multiple remotes.

git:
    remotes:
        origin: https://github.com/redhat-developer/devfile-sample
        my-fork: https://github.com/my-repo/devfile-sample

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The struct of remotes is a map

type Git struct {
	Remotes map[string]string `yaml:"remotes,omitempty" json:"remotes,omitempty"`
}

which means it is a map with multiple entries if the git has multiple remotes, so it's similar to your example above

Copy link
Member

Choose a reason for hiding this comment

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

I understand, but what is the expected index.. Will we have two index entries pointing to both the remote repositories? How does registry-support know which repo to choose in this case to point the sample/stack 🤔

The devfile/api schema has a checkoutFrom.remote to mention which remote if the length of remotes map is greater than 1:

Screen Shot 2021-05-05 at 2 12 34 PM

Copy link
Contributor Author

@GeekArthur GeekArthur May 6, 2021

Choose a reason for hiding this comment

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

For samples and stacks from other repos (resources not store in the registry), we only display the metadata to consumers from the index and they decide how to use them. Also I rethink this, given the git field is only for samples and stacks from other repos , then the relationship should be one specific sample/stack per specific repo, it's different from one generic stack contains multiple starter projects and multiple repos.

Copy link
Contributor

Choose a reason for hiding this comment

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

For sample projects, I don't think it should have multiple remote (same reason as starterProjects). Both samples and starterProjects are only being cloned one and then it gets detached from the original, i.e. no git remote is setup. Therefore, it doesn't make sense to have multiple remotes. We should restrict that use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To restrict it, I think we add it as part of the validation code in the index generator.

@openshift-ci
Copy link

openshift-ci bot commented May 10, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: GeekArthur, johnmcollier

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@GeekArthur GeekArthur merged commit 2312ff6 into devfile:main May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants