Skip to content
This repository has been archived by the owner on Aug 20, 2021. It is now read-only.

Set param #64

Merged
merged 1 commit into from
Jan 24, 2019
Merged

Set param #64

merged 1 commit into from
Jan 24, 2019

Conversation

minhaj10p
Copy link
Contributor

@minhaj10p minhaj10p commented Jan 24, 2019

Description

Adds processing for profile set-param in catalog code/xml/json generation

Dependencies

This PR to be merged after #58 having transitive dependency of #52

Copy link
Contributor

@asadullah-yousuf-10p asadullah-yousuf-10p left a comment

Choose a reason for hiding this comment

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

See my comment.

@@ -421,6 +422,53 @@ func TestProcessAdditionWithDifferentPartClass(t *testing.T) {
}

}

func TestProcessSetParam(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@minhaj10p can we write a negative test case as well to make sure that if a parameter isn't found in a prose, it shouldn't be changed after applying a profile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding negative test

Copy link
Contributor

@asadullah-yousuf-10p asadullah-yousuf-10p left a comment

Choose a reason for hiding this comment

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

LGTM

@minhaj10p
Copy link
Contributor Author

@anweiss This PR has been rebased as well now that you merged #58
Thanks for being proactive on merging that!

Copy link
Contributor

@anweiss anweiss left a comment

Choose a reason for hiding this comment

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

LGTM

@anweiss anweiss merged commit ff1c55b into docker-archive:master Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants