Adds dev.odo.push.path attribute support for pushing only mentioned files #4588
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
approved
mik-dass
changed the title
mik-dass
force-pushed
the
odo_push_dev
branch
2 times, most recently
from
074cfc7 to
f9ee1de
Compare
mik-dass
changed the title
mik-dass
force-pushed
the
odo_push_dev
branch
2 times, most recently
from
455dec1 to
c3b6667
Compare
pkg/sync/adapter.go
Outdated
| @@ -173,6 +170,37 @@ func (a Adapter) SyncFiles(syncParameters common.SyncParameters) (isPushRequired | |||
| return true, nil | |||
| } | |||
|
|
|||
| // TODO remove if no required | |||
There was a problem hiding this comment.
what does this mean?
There was a problem hiding this comment.
I meant if not required. Currently the PR is WIP so I am testing out certain things.
pkg/util/file_indexer.go
Outdated
| @@ -302,3 +307,311 @@ func WriteFile(newFileMap map[string]FileData, resolvedPath string) error { | |||
|
|
|||
| return err | |||
| } | |||
|
|
|||
| func RunIndexWithRemote(directory string, ignoreRules []string, remoteDirectories map[string]string) (ret IndexerRet, err error) { | |||
There was a problem hiding this comment.
I see common things between RunIndex and this function, can we consider writing a nice struct and follow a design pattern? maybe extract some functions from this..?
There was a problem hiding this comment.
I can suggest a structure, but tomorrow. Need to understand a bit of this code 🙂
There was a problem hiding this comment.
Current the PR is WIP and I am trying out certain things. Once ready, I will probably remove RunIndex.
There was a problem hiding this comment.
Are you planning to remove the RunIndex function? because still there seem to be common code between the functions?
There was a problem hiding this comment.
No, I am currently not planning to remove the RunIndex function. I was thinking of leaving it untouched for s2i components for now.
pkg/util/file_indexer.go
Outdated
| return ret, nil | ||
| } | ||
|
|
||
| func recursive(directory, srcBase, srcFile, destBase, destFile string, ignoreRules []string, remoteDirectories map[string]string, existingFileIndex FileIndex, fs filesystem.Filesystem) (IndexerRet, error) { |
There was a problem hiding this comment.
comments for this function? and also too many arguments passed, can we reduce them?
There was a problem hiding this comment.
Sure, I will reduce them.
pkg/util/file_indexer_test.go
Outdated
| @@ -243,3 +245,736 @@ func TestGenerateNewFileDataEntry(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func Test_recursive(t *testing.T) { | |||
| fs := filesystem.DefaultFs{} | |||
There was a problem hiding this comment.
why not consider using the memory mapped Fs here? afero one.
There was a problem hiding this comment.
The function currently uses filepath.Glob() which uses os.Lstat() internally. The library afero doesn't provide an implementation for filepath.Glob() as of now. Thus using the DefaultFs for now.
mik-dass
force-pushed
the
odo_push_dev
branch
3 times, most recently
from
89d4226 to
aefc708
Compare
mik-dass
force-pushed
the
odo_push_dev
branch
3 times, most recently
from
90bd5f1 to
5e71e7e
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mik-dass 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 |
mik-dass
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/test psi-kubernetes-integration-e2e |
mik-dass
changed the title
mik-dass
removed
the
do-not-merge/hold
|
I have done some testing with quarkus and it works great. I just noticed that if the local directory or file specified in annotation doesn't exist odo doesn't error out. |
I have changed the code to return an error in such an scenario. |
/retest |
|
/hold need to get this comment addressed #4588 (comment) |
openshift-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mik-dass 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 |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/lgtm |
openshift-ci
bot
added
the
lgtm
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@mik-dass: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
What type of PR is this?
/kind feature
What does does this PR do / why we need it:
It uses the
dev.odo.push.pathattribute to push only the mentioned files.Which issue(s) this PR fixes:
Fixes #3132
PR acceptance criteria:
Unit test
Integration test
Documentation : Adds a document regarding the odo.dev.push.path attributes in the devfile #4681
I have read the test guidelines
How to test changes / Special notes to the reviewer:
dev.odo.push.path:<local-path>:<remote-path>in the run command to mark the files/folders which should be pushed inside the container.odo pushkubectl execto check if the required file/folders are present inside the container and their location.