Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Move artifacts gathering around #3154

Merged
merged 4 commits into from
Jun 5, 2018
Merged

Move artifacts gathering around #3154

merged 4 commits into from
Jun 5, 2018

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented Jun 4, 2018

  • moves logging of node and pod info into artifact gathering in teardown
  • removes running diagnostics at least for now
  • avoid version check panic

Depends on #3152

@ghost ghost assigned 0xmichalis Jun 4, 2018
@ghost ghost added the in progress label Jun 4, 2018
@acs-bot acs-bot added the size/M label Jun 4, 2018
@codecov
Copy link

codecov bot commented Jun 4, 2018

Codecov Report

Merging #3154 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3154   +/-   ##
=======================================
  Coverage   52.24%   52.24%           
=======================================
  Files         102      102           
  Lines       15392    15392           
=======================================
  Hits         8042     8042           
  Misses       6624     6624           
  Partials      726      726

@0xmichalis
Copy link
Contributor Author

/assign jim-minter

@0xmichalis
Copy link
Contributor Author

/unassign

@jim-minter
Copy link
Member

/lgtm
/assign CecileRobertMichon
/unassign jim-minter

@CecileRobertMichon
Copy link
Contributor

@Kargakis could you please rebase? #3152 just got merged

@acs-bot acs-bot removed the lgtm label Jun 5, 2018
@ghost ghost assigned 0xmichalis Jun 5, 2018
@0xmichalis
Copy link
Contributor Author

Rebased

@0xmichalis
Copy link
Contributor Author

/label orchestrator/openshift

@0xmichalis
Copy link
Contributor Author

Flaked on #2952

@@ -153,6 +139,27 @@ func FetchLogs(kind, namespace, name string) string {
return string(out)
}

// FetchClusterInfo returns node and pod information about the cluster.
func FetchClusterInfo(logPath string) {
needsLog := map[string]string{
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 happy for this to merge, but you don't think map[string]func() error would be a better foundation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't feel strong either way, updated to your suggestion (which needs to be map[string]func() string).

Copy link
Member

Choose a reason for hiding this comment

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

apols, I meant map[string]func() (string, error) - in which case the changes to the other functions aren't needed and they're more reusable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, updated

@@ -174,7 +185,8 @@ func fetchControlPlaneLogs(distro, version, sshKeyPath, adminName, name, locatio
case common.OpenShiftVersionUnstable:
return fetchUnstableControlPlaneLogs(distro, sshKeyPath, sshAddress, name, logPath)
default:
panic(fmt.Sprintf("BUG: invalid OpenShift version %s", version))
log.Printf("Invalid OpenShift version %q - won't gather logs from the control plane", version)
return nil
Copy link
Member

Choose a reason for hiding this comment

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

as discussed - return fmt.Errorf() here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jim-minter
Copy link
Member

/lgtm
/unassign kargakis

@acs-bot acs-bot assigned jim-minter and unassigned 0xmichalis Jun 5, 2018
@acs-bot acs-bot added the lgtm label Jun 5, 2018
@jim-minter
Copy link
Member

/unassign jim-minter

@0xmichalis
Copy link
Contributor Author

@jackfrancis @CecileRobertMichon please merge

@jackfrancis
Copy link
Member

/approve

@acs-bot
Copy link

acs-bot commented Jun 5, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

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

@acs-bot acs-bot merged commit 0d6dd79 into Azure:master Jun 5, 2018
@ghost ghost removed the in progress label Jun 5, 2018
@0xmichalis 0xmichalis deleted the move-artifacts-gathering-around branch June 5, 2018 20:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants