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

Consume top level variables & attributes #82

Merged
merged 3 commits into from
May 3, 2021

Conversation

maysunfaisal
Copy link
Member

@maysunfaisal maysunfaisal commented Apr 23, 2021

Signed-off-by: Maysun J Faisal maysunaneek@gmail.com

What does this PR do?

  • Calls the devfile/api variable func to replace the top level variable key if present in the devfile
  • New interface func to get workspace content and the top-level attributes

What issues does this PR fix or reference?

Fixes devfile/api#351

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

New go tests

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
@maysunfaisal maysunfaisal marked this pull request as ready for review April 26, 2021 17:32
}

// UpdateAttributes updates the devfile top level attributes
func (d *DevfileV2) UpdateAttributes(attr attributes.Attributes) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

so update attributes is going to replace the entire top-level attribute?
or should just we update for a specific key?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I will look for the specfic key for update

} else if err == nil {
attributes, err := tt.devfilev2.GetAttributes()
if err != nil {
t.Errorf("TestUpdateAttributes error2 - %+v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

return after this error

wantAttributes: attributes.Attributes{}.PutString("key1", "value1").Put("key3", nestedValue, nil).PutString("key2", "value2"),
},
{
name: "If Schema 2.1.0 has an attribute already present, it should overwrite",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this is an expected behavior for add
if you check for other update & add functions for commands/components/projects etc., this should be an update behavior. for "add", if the key exist, should return an error?

Copy link
Member Author

@maysunfaisal maysunfaisal Apr 30, 2021

Choose a reason for hiding this comment

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

Will do these modifications:
update will look for a specific key, err out if absent
add will just add a new key, since attributes is a map if there is a key already present, it will just overwrite with the new value

Signed-off-by: Maysun J Faisal <maysunaneek@gmail.com>
@yangcao77
Copy link
Collaborator

Looks good to me

@yangcao77 yangcao77 added the lgtm label Apr 30, 2021
@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

@maysunfaisal maysunfaisal merged commit c8a6bbf into devfile:master May 3, 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.

Add library writer api to set top level attributes
3 participants