Skip to content
This repository was archived by the owner on Jun 14, 2019. It is now read-only.

Conversation

@droslean
Copy link
Member

@droslean droslean commented Oct 22, 2018

Adds the ci-operator.openshift.io/always-show-container-output annotation to specify which container logs, will be shown after the pod will fail.

closes #185

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 22, 2018
@droslean
Copy link
Member Author

/assign @bbguimaraes @petr-muller @stevekuznetsov

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2018
@petr-muller
Copy link
Member

I am confused about the intention of this change. I expected to find a different feature here! :)

My understanding is the following:

  1. In Add annotation to output logs for all containers. #184, we added a feature where, providing a boolean annotation, the users can force output from all containers to be present, no matter the exit code.
  2. @bbguimaraes requested changes in the feature and you filed Annotation to force log output containerwise. #185 for it
  3. This PR is supposed to close Annotation to force log output containerwise. #185 but to me, it looks this is a different feature.

Can you clarify? How is this related to #185? If it's unrelated, what is this feature (a different issue?).

@droslean
Copy link
Member Author

@bbguimaraes requested the changes, but in the end, it turned out that we will need the feature described in #184. This PR adds a new feature, described in #185.

TEMPLATES.md Outdated
Will output the logs of all the containers in the pod, no matter what the exit code was.
The value should be `true` to enable this feature.

`ci-operator.openshift.io/always-show-container-output`:
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if always-show-container-output is the right name for the annotation (the name was reused from the previous feature, right?). It may be misleading because it looks similar to the always-show-output one but works differently. I don't like the always part - it's not true (always != when pod fails and only if container terminated) and it is what makes it similar to always-show-output.

I am not sure about a good name, though... containers-logged-on-failure ?

TEMPLATES.md Outdated

`ci-operator.openshift.io/always-show-container-output`:
Will output the logs of the specified containers after the pod fails, if
they are in a terminated state.
Copy link
Member

Choose a reason for hiding this comment

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

Briefly describe the default state somewhere. What containers' output is captured by default?

TEMPLATES.md Outdated
they are in a terminated state.

**Note:**
To specify multiple containers in `ci-operator.openshift.io/wait-for-container-artifacts` and `ci-operator.openshift.io/always-show-container-output` annotations, the value should be splited by `,`.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps include this into the descriptions themselves? This would require rewritting all of them into a different form, but I think it would be worth the improvement.

Comma-separated list of container names for which ci-operator will wait until complete / for which ci-operator will collect and log their output if pod fails

Copy link
Member

@petr-muller petr-muller left a comment

Choose a reason for hiding this comment

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

I don't think the annotation name well represents the feature - we'll need to come up with something better.

Other comments are suggestions.

@petr-muller
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: droslean, petr-muller

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [droslean,petr-muller]

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

@openshift-merge-robot openshift-merge-robot merged commit 9f2220f into openshift:master Oct 23, 2018
@droslean droslean deleted the issue186 branch October 23, 2018 12:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Annotation to force log output containerwise.

6 participants