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

Change: Cluster setup exception message shortened #2055

Merged
merged 4 commits into from
Jul 2, 2024

Conversation

svteb
Copy link
Collaborator

@svteb svteb commented May 24, 2024

Description

As explained in the referenced issue, there was an unnecessary stack trace printed out in install_cluster_tools and uninstall_cluster_tools if ./cnf-testsuite setup has not been run. This was remediated by replacing the raise command with puts. Additionally I made the informative message more explicit for new users.

Issues:

Refs: #1926

How has this been tested:

  • Covered by existing integration testing
  • Added integration testing to cover
  • Verified all A/C passes
    • develop
    • master
    • tag/other branch
  • Test environment
    • Shared Packet K8s cluster
    • New Packet K8s cluster
    • Kind cluster
  • Have not tested

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 not work as expected)
  • Documentation update

Checklist:

Documentation

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • No updates required.

Code Review

  • Does the test handle fatal exceptions, ie. rescue block

Issue

  • Tasks in issue are checked off

@martin-mat martin-mat self-requested a review May 24, 2024 14:14
src/tasks/cluster_setup.cr Outdated Show resolved Hide resolved
src/tasks/cluster_setup.cr Outdated Show resolved Hide resolved
@martin-mat martin-mat self-requested a review May 27, 2024 14:20
Copy link
Collaborator

@martin-mat martin-mat left a comment

Choose a reason for hiding this comment

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

lgtm

@martin-mat martin-mat self-requested a review June 7, 2024 07:20
src/tasks/cluster_setup.cr Outdated Show resolved Hide resolved
src/tasks/cluster_setup.cr Outdated Show resolved Hide resolved
@svteb
Copy link
Collaborator Author

svteb commented Jun 11, 2024

I completely forgot to run spec tests, which even discovered the lack of exit code 1 as mentioned by @martin-mat . Additionally I had to edit the spec tests so that they would match the output with the new one.

spec/cluster_setup_spec.cr Outdated Show resolved Hide resolved
spec/cluster_setup_spec.cr Outdated Show resolved Hide resolved
spec/cluster_setup_spec.cr Outdated Show resolved Hide resolved
src/tasks/cluster_setup.cr Outdated Show resolved Hide resolved
src/tasks/cluster_setup.cr Outdated Show resolved Hide resolved
@martin-mat martin-mat requested review from martin-mat June 25, 2024 19:58
Copy link
Collaborator

@martin-mat martin-mat left a comment

Choose a reason for hiding this comment

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

lgtm

svteb added 4 commits June 26, 2024 08:23
Refs: cnti-testcatalog#1926
- The unnecessary stack trace was not printed through e.message as I suspected
but due to raise itself. That is why I instead used puts to print out the informative
message and the e.message remained in the form of an info log.
- I also made the informative message more explicit as someone not used to the testsuite
might not have understood it.

Signed-off-by: svteb <[email protected]>
@svteb svteb force-pushed the cluster_tools_exception branch from 0a4e2dd to 2c2fc12 Compare June 26, 2024 08:24
Copy link
Collaborator

@kosstennbl kosstennbl left a comment

Choose a reason for hiding this comment

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

LGTM, should make spec test output clearer and less confusing.
Pls add comment to PR about reason for removing Log.info { "install_cluster_tools" }

@svteb
Copy link
Collaborator Author

svteb commented Jun 28, 2024

LGTM, should make spec test output clearer and less confusing. Pls add comment to PR about reason for removing Log.info { "install_cluster_tools" }

The clustertools.install method prints that clustertools installation is ongoing:

  def self.install(host_namespace = true)
    Log.info { "ClusterTools install" }
    if host_namespace
      File.write("cluster_tools.yml", ManifestHostNamespaceTemplate.new().to_s)
    else
      File.write("cluster_tools.yml", ManifestTemplate.new().to_s)
    end
    KubectlClient::Apply.file("cluster_tools.yml", namespace: self.namespace!)
    wait_for_cluster_tools
  end

So just duplication removal.

Copy link

@rich-l rich-l left a comment

Choose a reason for hiding this comment

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

lgtm

@martin-mat martin-mat merged commit 2174f35 into cnti-testcatalog:main Jul 2, 2024
88 checks passed
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.

5 participants