Add UploadLogCommand #11
Conversation
| String tmp; | ||
|
|
||
| @CommandLine.Option(names = "--request-context", required = true) | ||
| String requestContext; |
There was a problem hiding this comment.
If I understand correctly this is uploading the single build.log that is saved in redhat-appstudio/jvm-build-service#2327 ?
So IMHO I think it would be better if the 'final upload' of both jars (etc) and the build.log is handled in the same command (i.e. https://github.com/project-ncl/konflux-tooling/blob/main/src/main/java/org/jboss/pnc/konfluxtooling/deploy/DeployCommand.java ) (whether or not is should be renamed is a separate discussion). Compressing them into the same java command means only a single tekton step action is being invoked and in future we can optimise in this single place.
I don't think the bifrost parameters should be required = true ; instead if they are not specified then no log upload should be performed? Also can we have some breakdown / comments / etc as to what they are?
Also did the PNC code have any tests to be ported across with it?
In terms of adding these new parameters to the pipeline they would be added all the way though I think i.e. the https://github.com/redhat-appstudio/jvm-build-service/blob/main/deploy/pipeline/mw-pipeline-v0.1.yaml , the deployment task, https://github.com/project-ncl/konflux-build-driver/blob/main/src/main/java/org/jboss/pnc/konfluxbuilddriver/Driver.java#L108 etc. And maybe the parameters (at least for now) should all be optional or have default values?
There was a problem hiding this comment.
I would go for a generic uploader that does not care about the content and can be used in any step, like wget or curl.
There was a problem hiding this comment.
@matejonnet I guess this code for uploading log is a bit specific for PNC. I don't know the details but looking at the those metadata properties would reveal that. Dustin also mentioned the current code has advanced features like streaming the file transfer. I'd suggest we stick to it.
@rnc I updated the code adding comments for those params. I don't know, at the moment, which one is required so I removed all 'required = true'. Also, as you suggested, skip it if bifrostURL is blank.
Regarding test, I didn't find unit test after looking around. I guess it needs a Bifrost server and we have to test it manually.
There was a problem hiding this comment.
@rnc In my viewpoint, it is better to separate the maven deployment with log upload. Although this means a new step in pipeline, it is easy to understand and maintain. As you can see, uploading log introduces a bunch of arguments. If we mix them to one command, it won't be easy to tell which is for which when people look at the pipeline args.
There was a problem hiding this comment.
I agree with both statements, keep the existing tool for log upload and keep the separate steps.
There was a problem hiding this comment.
@matejonnet I'm not convinced we need a generic tool - we already have the maven upload tooling but that is using the maven deployment code so isn't applicable. This tool is highly specific to PNC so how would the extra parameters be passed?
Further, I'm not convinced about keeping it separate - while it means the task parameters will be mixed it reduces the task step cpu/ memory cost (and bundle size for that matter) - I'm already seeing issues with running the buildah-oci task in some environments so I think we should reconsider this approach. Invoking as part of the deploy step will reduce the overall impact IMHO.
There was a problem hiding this comment.
Regarding cpu/memory, my personal experience is that it is a question of quota, something to be argued at later stage. Also, I see the current deployment step limits the cpu/mem to a minimal level. Maybe we don't need to worry too much about it at this moment?
There was a problem hiding this comment.
I'm sorry but I don't see the need to allocate another step with its own image initialisation/cpu/memory/processing time/etc giving we're only uploading a single file and could easily be rolled into the existing step. I think this is something we should all discuss in the new year.
| var file = logFilePath.toFile(); | ||
| if (!file.exists()) { | ||
| throw new RuntimeException(String.format( | ||
| "No log file found at %s. Has the build been correctly done?", logFilePath)); |
There was a problem hiding this comment.
Following on from https://github.com/project-ncl/konflux-tooling/pull/11/files#r1896576530 while currently the deployment directory is passed in https://github.com/redhat-appstudio/jvm-build-service/blob/main/deploy/tasks/maven-deployment.yaml#L104 but perhaps it should be the lower level workdir directory which should contain deployment and log directories next to each other?
There was a problem hiding this comment.
do you mean something like this?
/var/workdir/results
|-- deployment
|-- log
@matejonnet wdty?
There was a problem hiding this comment.
No just /var/workdir but this only makes sense if everything is in the same tool. Let's discuss in the new year when everyone is back :-)
src/main/java/org/jboss/pnc/konfluxtooling/logging/UploadLogCommand.java
Outdated
Show resolved
Hide resolved
|
/ok-to-test |
This is for JBS-72 Push build log to Bifrost