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

397 add annotation to imported elements #80

Merged

Conversation

yangcao77
Copy link
Collaborator

@yangcao77 yangcao77 commented Apr 16, 2021

What does this PR do?

This PR adds an attribute to all elements from the imported source before override & merge. This attribute information can be used by validator to track from which devfile the error came from.

What issues does this PR fix or reference?

devfile/api#397

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

Update unit tests. Add more unit tests to test the import attribute fields

Signed-off-by: Stephanie <[email protected]>
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.

could you also update the line here where we dont check for err when parsing from CRD:

Screen Shot 2021-04-20 at 1 19 31 PM

// AddSourceAttributesForParentOverride adds an attribute 'library.devfile.io/imported-from=<source reference>'
// to all elements of parent override that support attributes.
func AddSourceAttributesForParentOverride(sourceImportReference v1.ImportReference, parentoverride *v1.ParentOverrides) {
for idx, component := range parentoverride.Components {
Copy link
Member

Choose a reason for hiding this comment

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

should check if parentoverride is nil before using it, same for the others. Can we camel case to parentOverride?

Copy link
Collaborator Author

@yangcao77 yangcao77 Apr 20, 2021

Choose a reason for hiding this comment

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

updated. check the nil pointer in the top level function AddSourceAttributesForOverrideAndMerge as you suggested

}

}

// AddSourceAttributesForPluginOverride adds an attribute 'library.devfile.io/imported-from=<source reference>'
// addSourceAttributesForPluginOverride adds an attribute 'library.devfile.io/imported-from=<source reference>'
Copy link
Member

Choose a reason for hiding this comment

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

need to update func desc for the updated attribute key, same for the parent override func

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@maysunfaisal
Copy link
Member

maysunfaisal commented Apr 20, 2021

@yangcao77 Can you also check this screenshot? #80 (review)

@yangcao77
Copy link
Collaborator Author

@maysunfaisal #80 (review) has been fixed. the error is checked and returned outside of the switch case

@yangcao77 yangcao77 force-pushed the 397-addAnnotationToImportedElements branch from 41b592c to e3d1c45 Compare April 21, 2021 19:20
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maysunfaisal, yangcao77

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [maysunfaisal,yangcao77]

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

@yangcao77 yangcao77 merged commit ec1e90d into devfile:master Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants