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

MGMT-17770: Split ignition handling file #6283

Merged

Conversation

carbonin
Copy link
Member

@carbonin carbonin commented May 7, 2024

This PR splits the mostly unrelated logic in ignition/ignition.go into three files.

  • ignition/discovery.go: discovery ignition builder and helpers
  • ignition/installmanifests.go: runs installer binary to create host ignition files and cluster manifests
  • ignition/ignition.go: ignition parsing and a single common helper function

List all the issues related to this PR

https://issues.redhat.com/browse/MGMT-17770

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

It's a refactoring so unit tests and CI tests passing should be sufficient

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see [CONTRIBUTING] guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 7, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented May 7, 2024

@carbonin: This pull request references MGMT-17770 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set.

In response to this:

This PR splits the mostly unrelated logic in ignition/ignition.go into three files.

  • ignition/discovery.go: discovery ignition builder and helpers
  • ignition/installmanifests.go: runs installer binary to create host ignition files and cluster manifests
  • ignition/ignition.go: ignition parsing and a single common helper function

List all the issues related to this PR

https://issues.redhat.com/browse/MGMT-17770

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

It's a refactoring so unit tests and CI tests passing should be sufficient

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see [CONTRIBUTING] guide)
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 7, 2024
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2024
Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 60.52142% with 424 lines in your changes are missing coverage. Please review.

Project coverage is 68.37%. Comparing base (cb69eb2) to head (859bb96).
Report is 2 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6283      +/-   ##
==========================================
+ Coverage   68.25%   68.37%   +0.11%     
==========================================
  Files         242      244       +2     
  Lines       35902    35996      +94     
==========================================
+ Hits        24506    24611     +105     
+ Misses       9229     9218      -11     
  Partials     2167     2167              
Files Coverage Δ
internal/ignition/ignition.go 78.94% <94.59%> (+18.59%) ⬆️
internal/ignition/discovery.go 72.24% <72.24%> (ø)
internal/ignition/installmanifests.go 55.67% <55.67%> (ø)

... and 3 files with indirect coverage changes

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 12, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 904ee71 and 2 for PR HEAD d6519d5 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD fdf233a and 1 for PR HEAD d6519d5 in total

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 17, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2024
@gamli75
Copy link
Contributor

gamli75 commented May 21, 2024

/retest-required

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2024
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD fdf233a and 2 for PR HEAD cc4ecc5 in total

@carbonin
Copy link
Member Author

/retest-required

@gamli75
Copy link
Contributor

gamli75 commented May 22, 2024

/test edge-e2e-metal-assisted

@carbonin
Copy link
Member Author

/test e2e-agent-compact-ipv4

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD fad8782 and 1 for PR HEAD cc4ecc5 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD cb69eb2 and 0 for PR HEAD cc4ecc5 in total

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2024
Previously ignition.go contained code for general ignition parsing, code
for generating install manifests using opesnshift-install, and code for
creating the discovery ignition.

The discovery ignition and installer manifests code has mostly nothing
to do with each other so those should be separated out into their own
files.

ignition.go remains as a place for ignition parsing code as well as the
very small number of common functions between the installer manifests
and discovery ignition use cases.
This also makes some small improvements like moving some test globals
(like workDir, and cluster) into the per-test lifecyle which makes them
easier to track and manage if and when individual tests need them to
change.
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 22, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2024
@gamli75
Copy link
Contributor

gamli75 commented May 23, 2024

/test edge-subsystem-kubeapi-aws

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2024
Copy link

openshift-ci bot commented May 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, gamli75

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

openshift-ci bot commented May 23, 2024

@carbonin: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 901539c into openshift:master May 23, 2024
14 checks passed
@carbonin carbonin deleted the split-ignition-handling branch May 23, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants