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.

Sign up for GitHub

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

Jump to bottom

Implement odo log for devfiles #3384

Merged
merged 10 commits into from
Jul 9, 2020
Merged

Implement odo log for devfiles #3384

merged 10 commits into from
Jul 9, 2020

Conversation

adisky
Copy link
Contributor

@adisky adisky commented Jun 18, 2020 •
edited

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
Implement odo log for devfiles, It logs run command output.

Which issue(s) this PR fixes:

Fixes #3260

How to test changes / Special notes to the reviewer:
odo create nodejs
odo push
Run odo log to check the run command logs.
Run odo log --debug to check the debug command logs

@adisky adisky changed the title Implement odo log for devfiles [WIP] Implement odo log for devfiles Jun 18, 2020
@openshift-ci-robot openshift-ci-robot 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 Jun 18, 2020
@codecov
Copy link

codecov bot commented Jun 22, 2020 •
edited

Codecov Report

Merging #3384 into master will decrease coverage by 0.26%.
The diff coverage is 8.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3384      +/-   ##
==========================================
- Coverage   46.45%   46.18%   -0.27%     
==========================================
  Files         112      112              
  Lines       11237    11333      +96     
==========================================
+ Hits         5220     5234      +14     
- Misses       5513     5593      +80     
- Partials      504      506       +2
Impacted Files Coverage Δ
pkg/devfile/adapters/docker/component/adapter.go 63.08% <0.00%> (-10.93%) ⬇️
pkg/kclient/pods.go 38.13% <0.00%> (-6.42%) ⬇️
pkg/lclient/client.go 0.00% <ø> (ø)
pkg/lclient/mock_client.go 14.11% <0.00%> (-0.83%) ⬇️
pkg/occlient/occlient.go 50.44% <0.00%> (+0.38%) ⬆️
pkg/util/util.go 53.35% <0.00%> (-2.00%) ⬇️
...g/devfile/adapters/kubernetes/component/adapter.go 27.48% <4.76%> (-2.15%) ⬇️
pkg/lclient/containers.go 69.47% <37.50%> (-2.95%) ⬇️
pkg/lclient/fakeclient.go 82.69% <100.00%> (+0.57%) ⬆️
pkg/sync/adapter.go 82.35% <0.00%> (-0.53%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e367c33...65f5d0a. Read the comment docs.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jun 23, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jun 26, 2020
@adisky adisky changed the title [WIP] Implement odo log for devfiles Implement odo log for devfiles Jun 26, 2020
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. and 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 Jun 26, 2020
@girishramnani
Copy link
Contributor

@adisky this PR needs rebase

girishramnani
girishramnani reviewed Jul 2, 2020
View reviewed changes
pkg/devfile/adapters/docker/component/adapter.go Outdated

func (a Adapter) Log(follow, debug bool) (io.ReadCloser, error) {

exists, _ := utils.ComponentExists(a.Client, a.Devfile.Data, a.ComponentName)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am seeing this exists check in the beginning of every single adapter method

mik-dass
mik-dass reviewed Jul 7, 2020
View reviewed changes
pkg/devfile/adapters/docker/component/adapter.go Outdated

func (a Adapter) Log(follow, debug bool) (io.ReadCloser, error) {

exists, _ := utils.ComponentExists(a.Client, a.Devfile.Data, a.ComponentName)
Copy link
Contributor

@mik-dass mik-dass Jul 7, 2020 •
edited

Choose a reason for hiding this comment

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

we shouldn't ignore the error.

mik-dass
mik-dass reviewed Jul 7, 2020
View reviewed changes
pkg/devfile/adapters/docker/component/adapter.go Outdated

var command versionsCommon.DevfileCommand
if debug {
command, err = common.GetRunCommand(a.Devfile.Data, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

GetDebugCommand()

mik-dass
mik-dass reviewed Jul 7, 2020
View reviewed changes
pkg/kclient/pods.go Outdated

// If the log is being followed, set it to follow / don't wait
if followLog {
// TODO: https://github.com/kubernetes/kubernetes/pull/60696
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to have been merged a long time back. Has it been updated now?

mik-dass
mik-dass reviewed Jul 7, 2020
View reviewed changes
pkg/lclient/mock_client.go Outdated
Comment on lines 294 to 307
func (m *MockDockerClient) ContainerLogs(ctx context.Context, container string, options types.ContainerLogsOptions) (io.ReadCloser, error) {
return nil, nil
}
Copy link
Contributor

@mik-dass mik-dass Jul 7, 2020 •
edited

Choose a reason for hiding this comment

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

Is this auto generated by MockGen?

mik-dass
mik-dass reviewed Jul 7, 2020
View reviewed changes
pkg/odo/cli/component/log.go Outdated
}

// NewLogOptions returns new instance of LogOptions
func NewLogOptions() *LogOptions {
return &LogOptions{false, "", &ComponentOptions{}}
//return &LogOptions{false, "", &ComponentOptions{}, &envinfo.EnvSpecificInfo{}, ""}
Copy link
Contributor

Choose a reason for hiding this comment

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

comments found

mik-dass
mik-dass reviewed Jul 7, 2020
View reviewed changes
pkg/odo/cli/component/log.go Outdated
Comment on lines 36 to 37
// EnvSpecificInfo *envinfo.EnvSpecificInfo
// devfile path
Copy link
Contributor

Choose a reason for hiding this comment

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

comments found

@mik-dass
Copy link
Contributor

mik-dass commented Jul 7, 2020

Note: Added debug flag also odo log --debug which logs the output from debug command. since debug PR is in review, this flag does not work currently, would update this PR as soon as debug PR gets merged.

Debug is now merged.

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 7, 2020
@adisky
Copy link
Contributor Author

adisky commented Jul 7, 2020

@mik-dass Thanks for review, addressed all your comments.

mik-dass
mik-dass reviewed Jul 7, 2020
View reviewed changes
pkg/devfile/adapters/docker/component/adapter.go Outdated
return nil, err
}
if reflect.DeepEqual(versionsCommon.DevfileCommand{}, command) {
return nil, errors.Errorf("no debug command found in devfile, please run \"odo logs\" for run command logs")
Copy link
Contributor

@mik-dass mik-dass Jul 7, 2020 •
edited

Choose a reason for hiding this comment

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

odo log. odo logs leads to

 ✗  Invalid command - see available commands/subcommands above

mik-dass
mik-dass reviewed Jul 7, 2020
View reviewed changes
pkg/devfile/adapters/kubernetes/component/adapter.go
@@ -450,3 +451,40 @@ func (a Adapter) Delete(labels map[string]string) error {

return a.Client.DeleteDeployment(labels)
}

func (a Adapter) Log(follow, debug bool) (io.ReadCloser, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some comments for this function

@girishramnani
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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:

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

mik-dass
mik-dass approved these changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@mik-dass mik-dass left a comment

Choose a reason for hiding this comment

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

Works for me
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 8, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

mik-dass
mik-dass reviewed Jul 8, 2020
View reviewed changes
tests/integration/devfile/cmd_devfile_log_test.go
helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), context)

helper.CmdShouldPass("odo", "push", "--project", namespace)
helper.CmdShouldFail("odo", "log", "--debug")
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this test is failing
https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/3384/pull-ci-openshift-odo-master-v4.4-integration-e2e/1280783116180393984#1:build-log.txt%3A605

Copy link
Contributor

Choose a reason for hiding this comment

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

/hold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, failing as all devfiles in registry now have debug command

Copy link
Contributor

Choose a reason for hiding this comment

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

Still failing https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/3384/pull-ci-openshift-odo-master-v4.4-integration-e2e/1280783116180393984#1:build-log.txt%3A603 I guess it is still pulling the devfile from the registry

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. and removed lgtm Indicates that a PR is ready to be merged. Required by Prow. labels Jul 8, 2020
@adisky
Copy link
Contributor Author

adisky commented Jul 9, 2020 •
edited 

    /go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_create_test.go:230
    No future change is possible.  Bailing out early after 0.673s.
      
    Running odo with args [odo create nodejs --starter=nodejs-starter]
    Expected
        <int>: 1
    to match exit code:
        <int>: 0
    /go/src/github.com/openshift/odo/tests/helper/helper_run.go:34```

@adisky
Copy link
Contributor Author

adisky commented Jul 9, 2020

/retest

mik-dass
mik-dass reviewed Jul 9, 2020
View reviewed changes
.travis.yml
@@ -198,3 +198,4 @@ jobs:
- travis_wait make test-cmd-devfile-debug
- travis_wait make test-cmd-devfile-delete
- travis_wait make test-cmd-devfile-create
- travis_wait make test-cmd-devfile-log
Copy link
Contributor

Choose a reason for hiding this comment

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

please update the name of the test to include log as well

Update odo log debug test
65f5d0a 
Add log tests in travis
mik-dass
mik-dass approved these changes Jul 9, 2020
View reviewed changes
Copy link
Contributor

@mik-dass mik-dass left a comment

Choose a reason for hiding this comment

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

/lgtm

@mik-dass mik-dass removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Jul 9, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 9, 2020
@openshift-merge-robot openshift-merge-robot merged commit a9ebc8c into redhat-developer:master Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@girishramnani girishramnani girishramnani left review comments

@mik-dass mik-dass mik-dass approved these changes

@johnmcollier johnmcollier Awaiting requested review from johnmcollier

@mohammedzee1000 mohammedzee1000 Awaiting requested review from mohammedzee1000

Assignees

@mik-dass mik-dass

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

odo log for devfile component
6 participants
@adisky @girishramnani @mik-dass @openshift-ci-robot @openshift-bot @openshift-merge-robot