Implement new odo workflow for: odo watch #1446

cdrage · 2019-03-11T18:41:16Z

Implements odo watch by using the local configuration file.

To use these changes, simply use: odo watch in the same directory that
you've deployed, or odo watch --context ~/nodejs-ex.

This builds upon: #1360

Closes #1419

cdrage · 2019-03-11T18:44:58Z

A lot of TODOs:

Have to watch for Apply ComponentSettings as part of component push #1360 to be merged
Tests to be added
CLI validation (similar to @anmolbabu 's work on odo push validation) also needs to be added

P.S: This is WIP / draft! Silly how you can't convert from a mergable PR to draft after pushing... oh well!

codeclimate · 2019-03-11T18:45:19Z

Code Climate has analyzed commit 2c2a679 and detected 20 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	8
Duplication	4
Style	6
Bug Risk	2

View more on Code Climate.

cdrage · 2019-03-28T18:07:49Z

This is ready for review / testing / trying it out!

To test:

git clone https://github.com/openshift/nodejs-ex
odo create nodejs --context ~/nodejs-ex
odo push --context ~/node-js
odo watch --context ~/node-js

Same with --binary.

Examples below:

# Binary
▶ odo watch
Waiting for something to change in /home/wikus/example.war
File /home/wikus/example.war changed
Pushing files...
 ✓  Waiting for component to start
 ✓  Copying files to component
 ✓  Building component
Waiting for something to change in /home/wikus/example.war

# Local
▶ odo watch --context ~/nodejs-ex 
Waiting for something to change in /home/wikus/nodejs-ex
File /home/wikus/nodejs-ex/views/index.html changed
File /home/wikus/nodejs-ex/views/index.html~ changed
Pushing files...
Waiting for something to change in /home/wikus/nodejs-ex

cdrage · 2019-03-28T18:08:08Z

ping @anmolbabu @amitkrout @surajnarwade @girishramnani

amitkrout

My bad, could not realize the ping.

Can you please add a basic e2e watch test scenario as described in the pr description or you can update the commented scenario in the watch file

anmolbabu · 2019-04-04T14:43:14Z

pkg/odo/genericclioptions/context.go

+	SourcePath       string
+	LocalConfig      *config.LocalConfigInfo
+	ComponentContext string
+	Client           *occlient.Client


why client here again?
Almost every command generates context which already has client

anmolbabu · 2019-04-04T14:47:18Z

pkg/odo/genericclioptions/context.go

 )

+// ComponentOptions are generic options that are used between commands such as watch and push
+type ComponentOptions struct {


I don't see it used anywhere in this PR in the git diff view shown here
And why have Sourcetype, SourcePath etc here ?
Is there any more processing to these than what can be fetched from LocalConfig?

kadel · 2019-04-04T14:57:02Z

pkg/config/config.go

+// GetOSSourcePath corrects the current sourcePath depending on local or binary configuration,
+// if Git has been passed, we simply return the source location from LocalConfig
+// this will get the correct source path whether on Windows, macOS or Linux.
+func GetOSSourcePath(localConfig *LocalConfigInfo) (path string, err error) {


Can we align it with the rest of Get functions and use LocalConfig as a receiver?

func (lc *LocalConfig) GetOSSourcePath()

amitkrout · 2019-04-04T15:49:47Z

@amitkrout I already had an e2e_test.go created, but since we're refactoring e2e tests I won't be able to add it until #1535 is merged

Pr #1535 is merged, So please start adding watch test in new structure format.

@cdrage Can you pleas add the e2e test scenario in old format if you are not familiar enough with new test structure.

amitkrout · 2019-04-04T15:53:05Z

@cdrage Please have a look at CI failures

cdrage · 2019-04-04T16:09:34Z

Okay, will do @amitkrout 👍

cdrage · 2019-04-04T19:13:50Z

@amitkrout @kadel

So I've updated this PR again, if you could have another look at it...

@kadel I was thinking of instead using a separate PR to fix the default behaviour that we talked about with regards to local and binary components. Essentially, saving the path as ./ instead of the absolute path in the configuration..

As of right now, this PR works wonderfully with Windows and fixes the issues with odo watch.

A few things however:

e2e tests are proving to be very difficult as I'm uncommenting all the odo watch e2e tests and encountering wayyyyyy too many errors, could we open up an issue and I write some e2e tests after e2e test: core scenario for beta #1559 is merged in?
I'll open up another issue with regards to changing the default behaviour with ./ absolute directory being saved instead of the actual directory.

WDYT @kadel ?

Please check out the new updated function too! ^^

mik-dass

Code LGTM 👍 and tested on Linux and it works. But some e2e tests would be really good.

girishramnani

If this resolves the SourceLocation issue on windows then can we refer that too here @cdrage

syamgk

LGTM 👍

kadel · 2019-04-05T08:49:44Z

pkg/config/config.go

+	if sourceType == GIT {
+		glog.V(4).Info("Git source type detected, not correcting SourcePath location")
+		return sourceLocation, nil
+	}


👍 I like that you didn't put else here ❤️

amitkrout · 2019-04-05T08:52:38Z

e2e tests are proving to be very difficult as I'm uncommenting all the odo watch e2e tests and encountering wayyyyyy too many errors, could we open up an issue and I write some e2e tests after #1559 is merged in?

@cdrage No issue, please create an follow up issue for watch test only. Anyway the minimal watch test is handled by pr #1559 for beta release.

amitkrout

Approving this as the minimal watch test is taken care by #1559

syamgk · 2019-04-05T09:01:00Z

/approve

openshift-ci-robot · 2019-04-05T09:01:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: syamgk

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:

~~pkg/config/OWNERS~~ [syamgk]
~~pkg/odo/OWNERS~~ [syamgk]

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

mik-dass · 2019-04-05T09:03:15Z

/lgtm

girishramnani · 2019-04-05T09:05:56Z

/hold cancel

openshift-ci-robot requested review from amitkrout and anmolbabu March 11, 2019 18:41

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. size/XXL labels Mar 11, 2019

cdrage force-pushed the update-watch branch from bf6f5ed to 2c2a679 Compare March 11, 2019 18:42

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 11, 2019

cdrage 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 Mar 11, 2019

cdrage changed the title ~~Implement new odo workflow for: odo watch~~ [wip] Implement new odo workflow for: odo watch Mar 11, 2019

cdrage closed this Mar 11, 2019

cdrage reopened this Mar 11, 2019

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 13, 2019

cdrage changed the title ~~[wip] Implement new odo workflow for: odo watch~~ [WIP] Implement new odo workflow for: odo watch Mar 26, 2019

cdrage force-pushed the update-watch branch from 2c2a679 to 705661a Compare March 27, 2019 18:52

openshift-ci-robot added size/L and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. size/XXL labels Mar 27, 2019

cdrage force-pushed the update-watch branch from 705661a to 5b78164 Compare March 27, 2019 18:56

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Mar 27, 2019

cdrage mentioned this pull request Mar 27, 2019

config behavior according to new workflow #1526

Merged

cdrage force-pushed the update-watch branch from 5b78164 to 3ba2cab Compare March 28, 2019 17:49

cdrage changed the title ~~[WIP] Implement new odo workflow for: odo watch~~ Implement new odo workflow for: odo watch Mar 28, 2019

openshift-ci-robot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels Mar 28, 2019

cdrage force-pushed the update-watch branch from 3ba2cab to 100a359 Compare March 28, 2019 18:17

amitkrout suggested changes Mar 29, 2019

View reviewed changes

anmolbabu reviewed Apr 4, 2019

View reviewed changes

Make changes based on anmol's comments

027f4de

kadel reviewed Apr 4, 2019

View reviewed changes

Add changes based on tkral's comments

81075cb

Pass tests

b8e167c

cdrage mentioned this pull request Apr 4, 2019

e2e test: core scenario for beta #1559

Merged

More changes based on Tomas' input

cd1553a

cdrage force-pushed the update-watch branch from 061d27a to cd1553a Compare April 4, 2019 19:17

mik-dass approved these changes Apr 5, 2019

View reviewed changes

girishramnani reviewed Apr 5, 2019

View reviewed changes

syamgk approved these changes Apr 5, 2019

View reviewed changes

kadel reviewed Apr 5, 2019

View reviewed changes

amitkrout approved these changes Apr 5, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 5, 2019

openshift-ci-robot assigned mik-dass Apr 5, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Apr 5, 2019

openshift-merge-robot merged commit 2302e8d into redhat-developer:master Apr 5, 2019

cdrage deleted the update-watch branch January 14, 2022 14:58

rm3l added estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. and removed size/L labels Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new odo workflow for: odo watch #1446

Implement new odo workflow for: odo watch #1446

cdrage commented Mar 11, 2019

cdrage commented Mar 11, 2019 •

edited

Loading

codeclimate bot commented Mar 11, 2019

cdrage commented Mar 28, 2019 •

edited

Loading

cdrage commented Mar 28, 2019

amitkrout left a comment •

edited

Loading

anmolbabu Apr 4, 2019

anmolbabu Apr 4, 2019

kadel Apr 4, 2019

cdrage Apr 4, 2019

amitkrout commented Apr 4, 2019

amitkrout commented Apr 4, 2019

cdrage commented Apr 4, 2019

cdrage commented Apr 4, 2019

mik-dass left a comment

girishramnani left a comment

syamgk left a comment

kadel Apr 5, 2019

amitkrout commented Apr 5, 2019

amitkrout left a comment

syamgk commented Apr 5, 2019

openshift-ci-robot commented Apr 5, 2019

mik-dass commented Apr 5, 2019

girishramnani commented Apr 5, 2019

Implement new odo workflow for: odo watch #1446

Implement new odo workflow for: odo watch #1446

Conversation

cdrage commented Mar 11, 2019

cdrage commented Mar 11, 2019 • edited Loading

codeclimate bot commented Mar 11, 2019

cdrage commented Mar 28, 2019 • edited Loading

cdrage commented Mar 28, 2019

amitkrout left a comment • edited Loading

Choose a reason for hiding this comment

anmolbabu Apr 4, 2019

Choose a reason for hiding this comment

anmolbabu Apr 4, 2019

Choose a reason for hiding this comment

kadel Apr 4, 2019

Choose a reason for hiding this comment

cdrage Apr 4, 2019

Choose a reason for hiding this comment

amitkrout commented Apr 4, 2019

amitkrout commented Apr 4, 2019

cdrage commented Apr 4, 2019

cdrage commented Apr 4, 2019

mik-dass left a comment

Choose a reason for hiding this comment

girishramnani left a comment

Choose a reason for hiding this comment

syamgk left a comment

Choose a reason for hiding this comment

kadel Apr 5, 2019

Choose a reason for hiding this comment

amitkrout commented Apr 5, 2019

amitkrout left a comment

Choose a reason for hiding this comment

syamgk commented Apr 5, 2019

openshift-ci-robot commented Apr 5, 2019

mik-dass commented Apr 5, 2019

girishramnani commented Apr 5, 2019

cdrage commented Mar 11, 2019 •

edited

Loading

cdrage commented Mar 28, 2019 •

edited

Loading

amitkrout left a comment •

edited

Loading