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

feat(helm-test): Added helm test and linting #2003

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

amardeep2006
Copy link
Contributor

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This is related to #1975

  1. Added a workflow to lint and test helm charts
  2. Added maintainer info in Charts.yml
  3. Did one time linting for issues thrown by linter

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@diemol
Copy link
Member

diemol commented Nov 10, 2023

Seems the new action is failing.

@VietND96
Copy link
Member

In part of this PR, could you please update appVersion of Chart to be aligned with today image tag 4.15.0-20231110?

@VietND96
Copy link
Member

Warning FailedScheduling 5m default-scheduler 0/1 nodes are available: 1 Insufficient cpu. preemption: 0/1 nodes are available: 1 No preemption victims found for incoming pod..

Looks like the cluster doesn't have enough resource to start the default setup

@amardeep2006
Copy link
Contributor Author

amardeep2006 commented Nov 10, 2023

@diemol Please rerun the failed job, most probably GitHub runner ran out of computes while running k8s. Seems like intermittent issue. Happened with me as well once it worked on retry.
See the successful run on my forked repo : https://github.com/amardeep2006/docker-selenium/actions

@amardeep2006
Copy link
Contributor Author

In part of this PR, could you please update appVersion of Chart to be aligned with today image tag 4.15.0-20231110?

I just did and pushed the commit.

@VietND96
Copy link
Member

I saw the condition to trigger the workflow is on pull request.
Do we need to consider the combination of PR and contain changes in path charts/selenium-grid?

@amardeep2006
Copy link
Contributor Author

I saw the condition to trigger the workflow is on pull request. Do we need to consider the combination of PR and contain changes in path charts/selenium-grid?

This is handled in workflow. It first checks if there are some changes in helm chart and only then then executes the remaining steps.

image

@VietND96
Copy link
Member

@amardeep2006
Copy link
Contributor Author

Cool, how about if we handle on the trigger events, something like

on:
  pull_request:
    paths:
      - 'charts/selenium-grid/**'

I am referring to https://github.com/SeleniumHQ/docker-selenium/blob/trunk/.github/workflows/helm-chart-release.yml https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-your-pull_request-workflow-based-on-the-head-or-base-branch-of-a-pull-request

May be a separate PR as continuous improvement. It will take time to test new logic. The current if conditions are tested by me already.

@diemol
Copy link
Member

diemol commented Nov 13, 2023

Cool, how about if we handle on the trigger events, something like

on:
  pull_request:
    paths:
      - 'charts/selenium-grid/**'

I am referring to https://github.com/SeleniumHQ/docker-selenium/blob/trunk/.github/workflows/helm-chart-release.yml https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-your-pull_request-workflow-based-on-the-head-or-base-branch-of-a-pull-request

May be a separate PR as continuous improvement. It will take time to test new logic. The current if conditions are tested by me already.

I would prefer to have this already. Thanks!

@VietND96
Copy link
Member

While fixing another breakage, I have used your baseline to update trigger events, pull out configs for ct to a separate file, and add a test with multiple desired values files. PR #2009
The PR doesn't pick Lint update, so it failed. I commented out that step. I think will bring it back in your PR after merging.

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Thank you, @amardeep2006!

@diemol diemol merged commit 35a044a into SeleniumHQ:trunk Nov 14, 2023
4 checks passed
@VietND96 VietND96 linked an issue Nov 30, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm Charts maintainer needed
3 participants