Skip to content
This repository has been archived by the owner on Jul 23, 2024. It is now read-only.

Feat: add git prebuilt-tasks #300

Merged
merged 1 commit into from
May 9, 2023
Merged

Feat: add git prebuilt-tasks #300

merged 1 commit into from
May 9, 2023

Conversation

eloycoto
Copy link
Collaborator

@eloycoto eloycoto commented May 4, 2023

This commit addressed a new way to clone and archive git repositories inside Parodos.

The main objective for these tasks is to implement work on top of Move2kube.

@openshift-ci openshift-ci bot requested review from masayag and rgolangh May 4, 2023 09:24
masayag
masayag reviewed May 4, 2023
View reviewed changes
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchive.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchive.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchive.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchive.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchive.java Outdated

FileOutputStream out = new FileOutputStream(zipFile.toAbsolutePath().toString());
git.archive().setTree(repo.resolve("HEAD")).setFormat("zip").setOutputStream(out).call();

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Should the size be evaluated before?
  2. Should repo.resolve("HEAD") be parameterized and allow to be set by the dev?
SetTree - Set the tag, commit, or tree object to produce an archive for

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at this stage I do not think that it's needed, instead adding more complex can I create a followup issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure.

@eloycoto eloycoto force-pushed the GitTask branch 2 times, most recently from 40786b4 to 8a1cd7a Compare May 4, 2023 15:10
gciavarrini
gciavarrini reviewed May 5, 2023
View reviewed changes
prebuilt-tasks/pom.xml Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchive.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchive.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchive.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchive.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/git/GitArchiveTest.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/git/GitArchiveTest.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/git/GitCloneTest.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/git/GitCloneTest.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/git/GitCloneTest.java Outdated Show resolved Hide resolved
@eloycoto eloycoto force-pushed the GitTask branch 3 times, most recently from c380520 to f70f783 Compare May 8, 2023 08:25
@eloycoto
Copy link
Collaborator Author

eloycoto commented May 8, 2023

@gciavarrini regarding the try in test, is that ok?

If I have a test that does assertDoesNotTrhow, if I use the try inside it, it will not fail, no? Is it a valid test?

Regarding the multiple error types in the catch, At the end I would like to send a valid message to the user, is not the same as an IOException (File not found) than the Transport(Cannot connect, server without connection) and the InvalidRemote(can be invalid credentials)

I do not really like all these chain exceptions for multiple functions, but the style is what it is.

@eloycoto eloycoto force-pushed the GitTask branch 7 times, most recently from 1c77f71 to 16883d1 Compare May 8, 2023 10:14
@eloycoto
Copy link
Collaborator Author

eloycoto commented May 8, 2023 •
edited
Loading

@gciavarrini @masayag @pkliczewski @RichardW98 all the comments are now addressed; please review again and share your thoughts there.

pkliczewski
pkliczewski reviewed May 8, 2023
View reviewed changes
Copy link
Collaborator

@pkliczewski pkliczewski left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label May 8, 2023
masayag
masayag reviewed May 8, 2023
View reviewed changes
prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/git/GitCloneTaskTest.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitArchiveTask.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitCloneTask.java
return new DefaultWorkReport(WorkStatus.FAILED, workContext, e);
}
catch (TransportException e) {
return new DefaultWorkReport(WorkStatus.FAILED, workContext,
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps include the error message as well, if not logging the exception with DEBUG level.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll go with log, a error that said, cannot connect to 129.34.23.29 it's nothing that the user needs to know.

prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitCloneTask.java
}
catch (InvalidRemoteException e) {
return new DefaultWorkReport(WorkStatus.FAILED, workContext,
new Exception("Remote repository " + gitUri + " is not available"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps include the error message as well?

Copy link
Collaborator Author

@eloycoto eloycoto May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll go with log message as well, the error messages for these exceptions are developer-friendly, not user-friendly.

prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitCloneTask.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/main/java/com/redhat/parodos/tasks/git/GitUtils.java Outdated Show resolved Hide resolved
prebuilt-tasks/src/test/java/com/redhat/parodos/tasks/git/GitArchiveTaskTest.java Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm label May 8, 2023
@eloycoto eloycoto requested review from pkliczewski and masayag May 8, 2023 13:08
@eloycoto eloycoto force-pushed the GitTask branch 2 times, most recently from 6a53f26 to 24fc1d4 Compare May 8, 2023 14:31
@eloycoto eloycoto requested review from masayag and gciavarrini May 8, 2023 14:36
@eloycoto
Feat: add git prebuilt-tasks
c7147db 
This commit addressed a new way to clone and archive git repositories
inside Parodos.

The main objective for these tasks is to implement work on top of
Move2kube.

Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
@masayag
Copy link
Collaborator

masayag commented May 8, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label May 8, 2023
@gciavarrini
Copy link
Contributor

/approve

@openshift-ci
Copy link

openshift-ci bot commented May 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gciavarrini

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

@openshift-ci openshift-ci bot added the approved label May 9, 2023
@openshift-merge-robot openshift-merge-robot merged commit d7bd49b into rhdhorchestrator:main May 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rgolangh rgolangh Awaiting requested review from rgolangh

@RichardW98 RichardW98 Awaiting requested review from RichardW98

@pkliczewski pkliczewski Awaiting requested review from pkliczewski

@masayag masayag Awaiting requested review from masayag

@gciavarrini gciavarrini Awaiting requested review from gciavarrini

Assignees

@masayag masayag

@pkliczewski pkliczewski

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@eloycoto @masayag @gciavarrini @pkliczewski @RichardW98 @openshift-merge-robot