Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add build version to side nav #670

Merged

Conversation

rileyjbauer
Copy link
Contributor

@rileyjbauer rileyjbauer commented Jan 11, 2019

If the information is reachable, the side nav will now display the first 7 characters of the commit hash that the API server image was built from, and link to that commit.

Note to reviewers: hide whitespace changes


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rileyjbauer

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

1 similar comment
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rileyjbauer

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

Copy link
Contributor

@yebrahim yebrahim left a comment

Choose a reason for hiding this comment

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

LGTM, just minor comments.
/lgtm

expect(isCollapsed(tree)).toBe(false);

(window as any).innerWidth = narrowWidth;
const resizeEvent = new Event('resize');
window.dispatchEvent(resizeEvent);
expect(isCollapsed(tree)).toBe(false);
});

it('populates the display build information using the response from the healthz endpoint', async () => {
const buildInfo = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this doing something different than what's done in the beforeAll block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but I thought it would be better to show it explicitly here so it's clear where the expectations come from


expect(tree.state('displayBuildInfo')).toEqual(expect.objectContaining({
commitHash: 'unknown',
commitUrl: 'https://www.github.com/kubeflow/pipelines',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This part is being tested below too, did you mean to keep both tests? Maybe remove this line from here if so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Yep, removed

@Ark-kun
Copy link
Contributor

Ark-kun commented Jan 12, 2019

/test kubeflow-pipeline-build-image

@IronPan
Copy link
Member

IronPan commented Jan 12, 2019

related #590

@IronPan
Copy link
Member

IronPan commented Jan 12, 2019

/test kubeflow-pipeline-e2e-test

@rileyjbauer rileyjbauer force-pushed the add-system-version-to-side-nav branch from 2106c72 to 17961f0 Compare January 14, 2019 03:53
@yebrahim
Copy link
Contributor

/lgtm

@rileyjbauer rileyjbauer deleted the add-system-version-to-side-nav branch May 6, 2019 22:17
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
…low#670)

* For now, it looks like using multiple acm-repo (i.e. different
  repos for different clusters) will work better than trying
  to use a single acm-repo and cluster selectors.

  * I was hitting problems with the fact that Tekton can't be installed
    via ACM right now (it violates certain validation constraints).

  * I only wanted to install tekton on the auto-deploy cluster (kf-ci-v1)
    and not the kf-ci-management cluster but I was stil
    getting errors from ACM complaining the Tekton configs were invalid.

  * So as a work around I renamed acm-repo -> acm-repos with the intent
    we will have multiple subdirectories corresponding to different repos

  * A given cluster can then choose which cluster to enroll in

* Create a new cluster for the KF chatbot (see kubeflow/code-intelligence#142)

  * The cluster will be managed using CNRM and ACM on the kf-ci-management
    cluster. So we need to check in the configs.

  * The source (kustomize packages for the manifests) is in
    kubeflow/code-intelligence#145
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants