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

fix(gha): and circleci resource names #3914

Merged
merged 18 commits into from
Nov 24, 2022
Merged

Conversation

marynaKK
Copy link
Contributor

@marynaKK marynaKK commented Nov 22, 2022

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

  1. removed definitions from get_resource_types
  2. move functions related to parsing the resource name to base class of yaml_doc Runner
  3. removed duplicated function for finding start+end lines (also removed tests for this)
  4. GHA: fixed bug of resource names for jobs, on:
    'jobs.on' -> 'on.unsec33ure-worfklow'
    'jobs.jobs' -> 'jobs'
    
  5. circleCI: the checks that have entities with jobs, orbs were catching empty lines, for example, the line: ---. fixed to return only valid checks in this sense.
  6. circleCI: fixed resource names, examples of what we have now:
    jobs.test-docker-versioned-img.docker.image#2[postgres:14.2]
    jobs.test-curl-secret.steps.1[checkout]
    
    
  7. added resource names for circleCI IR too.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@marynaKK marynaKK marked this pull request as ready for review November 23, 2022 12:51
Comment on lines 79 to 90
def resolve_image_name(image_definition: dict[str, Any], start_line: int, end_line: int) -> str:
for idx, step in enumerate([step for step in image_definition.get('docker') or [] if step]):
if isinstance(image_definition.get('docker'), dict):
if step == 'image':
return f"{idx + 1}[{image_definition['docker'][step]}]"
if isinstance(step, str):
return f"{idx + 1}[{step}]"
elif isinstance(step, dict):
if step[START_LINE] <= start_line <= end_line <= step[END_LINE]:
name = step.get('image')
return f"{idx + 1}[{name}]" if name else str(idx + 1)
return ""
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the image index isn't too useful to the resource name

The index is included for steps in GHA since they might not have a name, but we will always have the image name

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 though it will be useful cause maybe someone will declare 2 images with different other parameters... WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the image resource id is defined as f'jobs.{job_name}.docker.image#{image_name}', as long as the same image appears on different jobs, the resource id will be distinct

Worth checking in circleci pipelines docs if using multiple images inside the same job is a valid use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

an example from circleCI documentation: https://circleci.com/docs/configuration-reference/#docker
I will leave it numbered :)

Copy link
Contributor

@Eliran-Turgeman Eliran-Turgeman left a comment

Choose a reason for hiding this comment

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

Great work & nice tests 🚀
Had a few comments

Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

nice job 💪 added a couple of comments

checkov/circleci_pipelines/image_referencer/provider.py Outdated Show resolved Hide resolved
checkov/circleci_pipelines/image_referencer/provider.py Outdated Show resolved Hide resolved
checkov/circleci_pipelines/runner.py Outdated Show resolved Hide resolved
checkov/circleci_pipelines/runner.py Outdated Show resolved Hide resolved
checkov/github_actions/runner.py Outdated Show resolved Hide resolved
checkov/yaml_doc/runner.py Show resolved Hide resolved
checkov/yaml_doc/runner.py Outdated Show resolved Hide resolved
checkov/yaml_doc/runner.py Show resolved Hide resolved
Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

🐶

Copy link
Contributor

@arielkru arielkru left a comment

Choose a reason for hiding this comment

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

Very nice!
Added comments about formatting, we should aim to a single formatting standard between the runners

checkov/circleci_pipelines/image_referencer/provider.py Outdated Show resolved Hide resolved
checkov/github_actions/runner.py Outdated Show resolved Hide resolved
Copy link
Contributor

@arielkru arielkru left a comment

Choose a reason for hiding this comment

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

🏅

@marynaKK marynaKK merged commit 787eedb into master Nov 24, 2022
@marynaKK marynaKK deleted the change-get-resources-func branch November 24, 2022 13:45
mayblo pushed a commit to cider-rnd/checkov that referenced this pull request Nov 24, 2022
* lots of changes

* small fix to docker resource naming

* removed logging

* more changes + image resource adduption

* fix mypy + flake8

* fixed checks

* added tests!

* added comment for future debug in case of changes

* Update checkov/circleci_pipelines/checks/image_version_not_hash.py

Co-authored-by: Anton Grübel <[email protected]>

* Update checkov/github_actions/runner.py

Co-authored-by: Anton Grübel <[email protected]>

* Update checkov/yaml_doc/runner.py

Co-authored-by: Anton Grübel <[email protected]>

* add docs test to yaml_doc functions

* added error logging

* add test for `sca_image_report.image_cached_results`

* fix tests of merge of master

* changed resource naming

Co-authored-by: Anton Grübel <[email protected]>
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.

4 participants