Remove ofcourse 4: put part 1 #88
Conversation
This is the absolute minimum to have a skeleton executable for /opt/resource/out and passing tests. We will build upon this commit. PCI-2587
This is needed to be able to run build and tests successfully. At the end of this branch, all the code in the "resource" pkg will be migrated to the "cogito" pkg, thus all tests and functionalities will be back. PCI-2587
While there, finally found an easy way to test that the executable, when built with Task, contains the build info. PCI-2587
We now perform more logic in processInputDir() instead of doing it directly in Put(). Since tests involve the file system, this allows to keep the tests for Put() slightly simpler and higher-level. PCI-2587
marco-m-pix4d
requested review from
aliculPix4D,
Esysc,
iAmoric and
odormond
as code owners
There was a problem hiding this comment.
Expanding the large diff of resource/resource.go I notices that github is showing some errors:
(it's not the only ones and there is more on untouched files at the end of the PR: resource/chatadapter.go and resource/commitstatusadapter.go)
Are these things to worry about?
| // From https://concourse-ci.org/implementing-resource-types.html#resource-out: | ||
| // | ||
| // The out script is passed a path to the directory containing the build's full set of | ||
| // sources as command line argument $1, and is given on stdin the configured params and |
There was a problem hiding this comment.
| // sources as command line argument $1, and is given on stdin the configured params and | |
| // inputs as command line argument $1, and is given on stdin the configured params and |
Because what you get in that directory is what is defined by the inputs and input_mapping while the term "source" is used for the base configuration of the resource. So I think using sources here is misleading.
There was a problem hiding this comment.
Ah yes. Actually this is copied from the Concourse docs, but I agree it is wrong. Will update accordingly.
- fix comment
There was a problem hiding this comment.
@odormond updated in the last PR of this series, thanks.
| @@ -27,7 +27,8 @@ func IdentityRenamer(name string) string { | |||
| return name | |||
| } | |||
|
|
|||
| // Recursive copy src directory below dst directory, with optional transformations. | |||
| // CopyDir recursively copies src directory below dst directory, with optional | |||
There was a problem hiding this comment.
There is not much point in using this to just copy a directory so maybe it would be better named templatedir or fleshoutdir?
There was a problem hiding this comment.
To give some background:
I seem to remember that I called it (at the time of Cogito creation, this is just moved) CopyDir because there is no function to copy a directory in Go stdlib, so if it is invoked as
CopyDir with IdentityRenamer and with empty templatedata, it will actually behave as a plain CopyDir.
Having said this, I am still open to renaming, or maybe making this parametric function as private and exposing two public functions, with clearer names and more focused argument list ?
There was a problem hiding this comment.
Discussion proceeds in comment thread below #88 (comment)
This is what is mentioned in the PR description:
Just ignore them. Final PR will ensure not to have any. |
OK but what about the ones in |
| ./bin/copydir cogito/testdata/one-repo/a-repo /tmp/cogito-test --dot | ||
| --template repo_url=https://github.com/foo/bar head=dummyHead | ||
| branch_name=dummyBranch commit_sha=dummySha |
There was a problem hiding this comment.
@marco-m-pix4d, this is actually the line that triggered my comment on renaming copydir. I didn't think of it being used as a function in go code where a std lib copy isn't available. Just renaming the binary would be enough, IMO.
There was a problem hiding this comment.
Ah, good point. Suggestions for the name of this executable? Same as in the previous comment: templatedir or fleshoutdir?
There was a problem hiding this comment.
Yes. I prefer templatedir.
There was a problem hiding this comment.
@odormond
addressed in last PR of the series.
The idea I had was to remove from resource/resource.go incrementally what I was migrating in this series of PR. I think that I will do the following:
Before doing this last resort, I will try to modify resource/resource.go to still keep around what I need and not trigger this github "feature". To directly answer the question
The integration of the two sinks: github commit status and ghcat. |
This PR adds a not-yet-complete new /opt/resource/out executable implementing the put step, arriving to the point of processing the "put input" directory containing the git repo and extracting the git commit hash.
Mostly it migrates existing code and existing tests and testdata from directory cogito/resource to directory cogito/cogito, adapting the code and the tests to the new approach.
While doing the migration, I also slightly simplified the tests and increased test code coverage. Currently we have 100% test code coverage for all the steps.
I also added new Taskfile targets:
test:smoke, that does a smoke test of the 3 executables.test:buildinfothat verifies that the executable contains the build information added at link time.Better reviewed commit-per-commit.
Part of PCI-2587.
EDIT: please ignore, at the bottom of the PR, the section added by GitHub with name Unchanged files with check annotations. That code is currently unused and has not been migrated yet, it does not impact tests nor build.