Complete more of the move from certification to internal

[bcrochet]

• Moves certification/engine
• Moves certification/formatters
• Leaves the FormatterFunc type exported
• Moves certification/pyxis
• Moves certification/runtime
• Moves certification/policy
• Creates runtime top-level package

Signed-off-by: Brad P. Crochet [email protected]

[dcibot]

• SUCCESS https://www.distributed-ci.io/jobs/e3eba70f-66c3-4f36-9c5c-e48898ab2c90/jobStates
  from Complete more of the move from certification to internal #840

[komish]

As a library caller, this PR would not allow me to extract artifacts, such as the preflight.log, from my execution. The artifacts package needs to be made available so that a library caller can manipulate the context with the appropriate ArtifactWriter.

[bcrochet]

How would a library caller retrieve their artifacts if the artifact writer definitions are all internal

Should library users wish to retrieve the artifacts, I would propose that a (future) API could be added to reach internal and get a tar to get the artifacts. I currently view artifacts as a preflight-specific implementation detail, and not a general purpose thing for library users.

[komish]

Should library users wish to retrieve the artifacts, I would propose that a (future) API could be added to reach internal and get a tar to get the artifacts. I currently view artifacts as a preflight-specific implementation detail, and not a general purpose thing for library users.

This doesn't work for me. If a library caller gets an error or failing result, (i.e. they don't pass), they have nothing they can do to troubleshoot the problem.

I don't see it as strictly internal functionality because of this. I also don't see it as a CLI-specific implementation detail.

[komish]

I also don't see a reason to remove the formatters. The API isn't complicated, and it does a single thing - and guides developers on how they can implement their own.

With this said, I don't think it's strictly necessary. If you want it removed, you'll need to update doc/LIBRARY.md to remove the reference to it.

[dcibot]

• SUCCESS https://www.distributed-ci.io/jobs/feba32f1-9908-47c5-b9f7-2dbaae66e704/jobStates
  from Complete more of the move from certification to internal #840

[bcrochet]

Should library users wish to retrieve the artifacts, I would propose that a (future) API could be added to reach internal and get a tar to get the artifacts. I currently view artifacts as a preflight-specific implementation detail, and not a general purpose thing for library users.

This doesn't work for me. If a library caller gets an error or failing result, (i.e. they don't pass), they have nothing they can do to troubleshoot the problem.

I don't see it as strictly internal functionality because of this. I also don't see it as a CLI-specific implementation detail.

I think I can meet you halfway here. While I don't think the current implementation is necessary to be exposed, I do think that some mechanism to retrieve the artifacts would be useful to a library user. So, what I would propose is moving forward here, but then being thoughtful about what API we want to expose. Does that work for you? I do agree that the library user should have the opportunity to review them, I just don't think they necessarily need to write them, which is more what this artifacts implementation currently does.

[bcrochet]

I also don't see a reason to remove the formatters. The API isn't complicated, and it does a single thing - and guides developers on how they can implement their own.

With this said, I don't think it's strictly necessary. If you want it removed, you'll need to update doc/LIBRARY.md to remove the reference to it.

I can come along with this. I'll pull them back out.

[dcibot]

• SUCCESS https://www.distributed-ci.io/jobs/feabcf04-a736-43ad-887b-fc3e39c238be/jobStates
  from Complete more of the move from certification to internal #840

[komish]

@bcrochet

Re: Artifacts

So, what I would propose is moving forward here, but then being thoughtful about what API we want to expose

This is fine - but we've got no roadmap planned on this. So, what do we do to bridge the gap until then?

I think I would have less of an issue with moving the artifacts into an internal package if executing Preflight (either policy) and having a failing or erroring result could be debugged without it. But as of today, the results for certain checks themselves tell the user top refer to the artifacts for information on how to troubleshoot.. Let alone the embedding of the preflight.log as an artifact for library callers.

I do agree that the library user should have the opportunity to review them, I just don't think they necessarily need to write them, which is more what this artifacts implementation currently does.

While some of the implementation makes reference to "Write", It's really just a semantic. We can call these methods anything we want. The FilesystemWriter "writes" to the fs, but the MapWriter is just building a map. The WriteFile method in the ArtifactWriter interface just maintains the verbiage used historically. IIRC, the previous artifacts directory had the same function.

---

Re: Formatters

I know you said you'd pull these back out. I'll propose an alternative path - realistically, we don't care what people do with the formatted results, but our ResponseFormatter gives describes extra functionality such as the extension that we don't necessary need to expose.

If we want to limit our API surface, maybe we just expose the FormatterFunc and a few functions such as FormatJUnitXML(...) FormatJSON(...) that effectively calls our internal type's FormatterFuncs? Or even just the DefaultFormatter.

The surface would be just that one type definition and the function(s) that we want to provide as, sort of, "defaults". That way, we encourage folks to write their own FormatterFunc, and we can keep our interface, implementations, internal to be modified.

[acornett21]

I'm not sure why we changed this implementation detail, and added a String() method in runtime.go. Does this buy us something I'm not seeing?

[acornett21]

Also, this doesn't seem to work. This is always unknown/unknown in the json that gets written to disk and displayed to the user, even though internal results struct is correct internally pre-formatter call. I assume it's the genericFormatter that's causing the issue, but didn't look further.

[bcrochet]

I'm not sure why we changed this implementation detail, and added a String() method in runtime.go. Does this buy us something I'm not seeing?

There was no reason to expose the OpenshiftClusterVersion struct. I'll admit that where it stands right now is completely naive and may (probably?) doesn't work as it stands. I haven't looked at it further either.

[acornett21]

We do 'expose' OpenshiftClusterVersion struct though, in the sense that it is written to disk, and written to the console. And as it stands that does not work for the reasons I stated above.

[bcrochet]

@bcrochet

Re: Artifacts

So, what I would propose is moving forward here, but then being thoughtful about what API we want to expose

This is fine - but we've got no roadmap planned on this. So, what do we do to bridge the gap until then?

I think I would have less of an issue with moving the artifacts into an internal package if executing Preflight (either policy) and having a failing or erroring result could be debugged without it. But as of today, the results for certain checks themselves tell the user top refer to the artifacts for information on how to troubleshoot.. Let alone the embedding of the preflight.log as an artifact for library callers.

As I proposed elsewhere, I think the API to artifacts should just be a 'SaveXXX', like 'SaveTar', or 'SaveFs', 'SaveDir'. Or, 'Save(opt ArtifactOption)'. I don't think the writing of artifacts is something that the library user should be relying on the library to do. If a library user wants to add artifacts, they do so to their own artifacts payload without having to reach into the library to do it. Now, to GET the artifacts that the library generates, that makes sense.

I do agree that the library user should have the opportunity to review them, I just don't think they necessarily need to write them, which is more what this artifacts implementation currently does.

While some of the implementation makes reference to "Write", It's really just a semantic. We can call these methods anything we want. The FilesystemWriter "writes" to the fs, but the MapWriter is just building a map. The WriteFile method in the ArtifactWriter interface just maintains the verbiage used historically. IIRC, the previous artifacts directory had the same function.

I get that, however, I don't think it's necessary to expose that from the library. As I previously stated, I think the library should provide the capability to get to the artifacts generated, but not be responsible for writing to the library user's artifacts.

[bcrochet]

Re: Formatters

I know you said you'd pull these back out. I'll propose an alternative path - realistically, we don't care what people do with the formatted results, but our ResponseFormatter gives describes extra functionality such as the extension that we don't necessary need to expose.

If we want to limit our API surface, maybe we just expose the FormatterFunc and a few functions such as FormatJUnitXML(...) FormatJSON(...) that effectively calls our internal type's FormatterFuncs? Or even just the DefaultFormatter.

The surface would be just that one type definition and the function(s) that we want to provide as, sort of, "defaults". That way, we encourage folks to write their own FormatterFunc, and we can keep our interface, implementations, internal to be modified.

I can get on board with this. Expose our current formatters that we've implemented, but still allow for creating your own formatter.

This tells me that these should probably be separate PRs.

[komish]

I can get on board with this. Expose our current formatters that we've implemented, but still allow for creating your own formatter.

This tells me that these should probably be separate PRs.

However you want to do this is fine by me.

[komish]

I get that, however, I don't think it's necessary to expose that from the library. As I previously stated, I think the library should provide the capability to get to the artifacts generated, but not be responsible for writing to the library user's artifacts.

Maybe we're not in the seeing each other regarding this point. The ArtifactWriter isn't something that the library caller consumes. It's effectively a choice the library caller makes, telling preflight how to write its artifacts in a way that the library caller can consume. "Write my artifacts here, and I'll retrieve them later".

As an example, if a library caller wanted to... say write an S3 Uploader ArtifactWriter - they could do that, then embed the artifact writer in the context and preflight would write its artifacts however the caller wishes.

[dcibot]

• SUCCESS https://www.distributed-ci.io/jobs/39d44cb6-9c80-4d0f-bb08-e202f3887db6/jobStates
  from Complete more of the move from certification to internal #840

[dcibot]

• SUCCESS https://www.distributed-ci.io/jobs/bc283483-3a41-45dd-ae06-795a8a0d69b0/jobStates
  from Complete more of the move from certification to internal #840

[komish]

Just to close the discussion loop here, I believe we landed on leaving the artifacts implementation on this branch as is for the moment, while @bcrochet reaches an alternative implementation that may be a bit more clearly defined for our use case.

The artifacts implementation as it exists in this branch would, therefore, move to the top level and outside of the certification module (as it is not needed).

Regarding logging, I believe we landed on exposing a few of our formatting functions as opposed to the internal formatter logic itself. Formatting, in itself, can be done however the user sees fit, and so we only intend to provide the execution our results formatting that we use as a baseline

@bcrochet please correct me if I've captured this incorrectly, it's been several days since our discussion!

[komish]

Feel free to ignore - does this still need to be here? It just looks like an alias, but I'm not sure why we would have used it.

[bcrochet]

It's an alias that reaches into internal so it can still be used as a type here, without having to duplicate it, and so that it isn't exposed as an actual part of the API.

[komish]

/lgtm

Thanks for this heavy lift!

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, bcrochet, komish

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:

• OWNERS [acornett21,bcrochet,komish]

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