Use example from the repo instead of downloading it #4207

dharmit · 2020-11-10T10:02:53Z

What type of PR is this?
/kind cleanup

What does this PR do / why we need it:
We need this to remove external dependency of downloading the code from GitHub each time in the test

Which issue(s) this PR fixes:
Fixes #3020

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:
Tests should pass.

girishramnani · 2020-11-10T10:15:49Z

Sub modules are hard to maintain, are you sure about it?

girishramnani · 2020-11-10T10:16:34Z

We would need to add a git submodule init & sync step in the readme

dharmit · 2020-11-10T10:25:07Z

Sub modules are hard to maintain, are you sure about it?

I'm not. That's why I am seeking thoughts from others.

We would need to add a git submodule init & sync step in the readme

I think it's more like we'll have to sync if there's any change in the submodule repo that we want to use on our side. But I get what you're suggesting. It's not as straightforward as, maybe, cloning the code somewhere and doing a cp.

EDIT: I would, personally, put maintainability of the code above everything else. If we're not comfortable maintaining something in the long run, we should avoid it and make it more maintainable.

adisky · 2020-11-10T11:29:22Z

We are storing some external dependencies here https://github.com/openshift/odo/tree/master/tests/examples/source/devfiles/nodejs

We can add this one too, but i am not sure how maintainable it is to store all dependencies in odo.

adisky · 2020-11-10T11:37:41Z

This issue is reported before we started caching devfile registry, then it could be due to hitting github API rate limits.
have we seen this issue recently?

kadel · 2020-11-10T11:42:11Z

Sub modules are hard to maintain, are you sure about it?

I don't think that there is any reason why it needs to be a submodule. We can simply copy the samples we are already doing that it some examples.

codecov · 2020-11-10T13:19:07Z

Codecov Report

Merging #4207 (97472cf) into master (fce3c77) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4207   +/-   ##
=======================================
  Coverage   42.57%   42.57%           
=======================================
  Files         156      156           
  Lines       13258    13258           
=======================================
  Hits         5645     5645           
  Misses       7012     7012           
  Partials      601      601

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 fce3c77...97472cf. Read the comment docs.

dharmit · 2020-11-10T14:19:55Z

It looks like we're unanimously sceptical of using submodule. 😄
I've removed the submodule thingy and simply copied the code for the repo.

adisky · 2020-11-11T07:11:57Z

/lgtm

tests/integration/devfile/cmd_devfile_log_test.go

mohammedzee1000 · 2020-11-12T05:18:39Z

It might also be a good idea to add a script that updates the examples from its source, for when we need to.

dharmit · 2020-11-12T07:03:57Z

It might also be a good idea to add a script that updates the examples from its source, for when we need to.

That would be over engineering things here, IMHO.

mik-dass

/lgtm

girishramnani · 2020-11-12T16:47:52Z

/approve

openshift-ci-robot · 2020-11-12T16:48:13Z

[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:

~~tests/OWNERS~~ [girishramnani]

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

openshift-bot · 2020-11-12T21:13:27Z

/retest