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

Draft: Ask for Devfile Personalization #5458

Closed
wants to merge 10 commits into from

Conversation

valaparthvi
Copy link
Contributor

@valaparthvi valaparthvi commented Feb 14, 2022

What type of PR is this:
/kind feature

What does this PR do / why we need it:
This PR allows user to personalize their devfile by adding/removing port, and environment variable.
Which issue(s) this PR fixes:

Fixes #5423

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:
The devfile/library vendor files have been modified until devfile/library#131 is merged.

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Feb 14, 2022
@netlify
Copy link

netlify bot commented Feb 14, 2022

✔️ Deploy Preview for odo-docusaurus-preview canceled.

🔨 Explore the source changes: 331dc19

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/620b972b0bfa8500075bad0f

@openshift-ci openshift-ci bot requested review from dharmit and feloy February 14, 2022 12:43
@valaparthvi valaparthvi changed the title Ask for Devfile Personalization Draft: Ask for Devfile Personalization Feb 14, 2022
@valaparthvi valaparthvi marked this pull request as draft February 14, 2022 12:43
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Feb 14, 2022
@odo-robot
Copy link

odo-robot bot commented Feb 14, 2022

Validate Tests on commit eecedef finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 14, 2022

Unit Tests on commit eecedef finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 14, 2022

OpenShift Tests on commit eecedef finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 14, 2022

Kubernetes Tests on commit eecedef finished with errors.
View logs: TXT HTML

pkg/init/init.go Outdated
@@ -184,3 +189,176 @@ func (o *InitClient) PersonalizeName(devfile parser.DevfileObj, flags map[string
err := backend.PersonalizeName(devfile, flags)
return err
}

func (o *InitClient) PersonalizeDevfileConfig(devfileobj parser.DevfileObj) error {
var envs = map[string]string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

In this pkg/init package, there should not be specialized code, only calls to backends. You will need to place this code into the interactive package, and call it from here (at this point, the only choice is to personalize the devfile config interactively, but it could change)

pkg/init/init.go Outdated
for configChangeAnswer != "NOTHING - configuration is correct" {
printConfiguration(portsMap, envs)

configChangeQuestion := &survey.Select{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create functions on the asker package to handle the survey specific operations. You can imagine a struct to make the interface between this function and the function into Asker, with something similar to:

  • an operation (end, add, delete)
  • a kind (port, envvar)
  • a key
  • a value

This way:

  • it will be possible to mock the asker method and test the logic of this function.
  • you will be able to test on some code (the operation of your struct) instead of literal values ("Delete port", etc)

pkg/init/init.go Outdated
}

if strings.HasPrefix(configChangeAnswer, "Delete port") {
re := regexp.MustCompile("\"(.*?)\"")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't rely on literal values (as UX people would like to change these without breaking the code).
Instead, Survey is able to reply with the index of the selected value, you can rely on this index to know the initial values without analysing the literal value (you can have two arrays in the same order, one with the containerName / port, the other with the literal value Delete port (container: "runtime") )

@odo-robot
Copy link

odo-robot bot commented Feb 14, 2022

Windows Tests (OCP) on commit eecedef finished with errors.
View logs: TXT HTML

@openshift-ci
Copy link

openshift-ci bot commented Feb 15, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from feloy after the PR has been reviewed.

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

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.4% 0.4% Duplication

@valaparthvi
Copy link
Contributor Author

/close
In favor of #5467

@openshift-ci openshift-ci bot closed this Feb 17, 2022
@openshift-ci
Copy link

openshift-ci bot commented Feb 17, 2022

@valaparthvi: Closed this PR.

In response to this:

/close
In favor of #5467

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ask for devfile personalization
2 participants