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

Kueue preparation #768

Merged
merged 15 commits into from
Jan 8, 2025
Merged

Kueue preparation #768

merged 15 commits into from
Jan 8, 2025

Conversation

at88mph
Copy link
Member

@at88mph at88mph commented Jan 7, 2025

Description

This pull request ports the refactored Java from PR #719.

Changes Included

  • Introduce KubectlCommandBuilder class.
  • Modified core components to use KubectlCommandBuilder.
  • Ensured backward compatibility.

Purpose

This refactoring includes helpful changes to isolate the kubectl commands.

Testing

  • Unit tests ran
  • Ran integration tests to confirm no regressions.
  • Performed end-to-end testing.

Related Issues/PRs

Checklist

  • Tests added or updated to cover new functionality.
  • Code reviewed for adherence to best practices and coding standards.

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.60%. Comparing base (f5dd456) to head (15056a8).
Report is 16 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #768      +/-   ##
============================================
+ Coverage     18.61%   20.60%   +1.99%     
- Complexity      102      118      +16     
============================================
  Files            22       23       +1     
  Lines          1961     1980      +19     
  Branches        270      270              
============================================
+ Hits            365      408      +43     
+ Misses         1544     1521      -23     
+ Partials         52       51       -1     
Flag Coverage Δ
skaha-unittests-coverage 20.60% <ø> (+1.99%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@at88mph at88mph requested a review from shinybrar January 8, 2025 20:55
Copy link
Collaborator

@shinybrar shinybrar left a comment

Choose a reason for hiding this comment

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

Most of these changes look good, except we need significant documentation, since we are touching so many files.

To enable docstrings checks, following edits are needed

// build.gradle
spotless {
  java {
    // Interpret all files as utf-8
    encoding 'UTF-8'
    // Only require formatting of files that diff from main
    ratchetFrom 'origin/main'
    // Use the default importOrder configuration
    importOrder()
    // Remove unused imports
    removeUnusedImports()
    // Google Java Format, Android Open Source Project style which uses 4 spaces for indentation
    palantirJavaFormat('2.50.0').formatJavadoc(true)
    // Format annotations on a single line
    formatAnnotations()
    // Checks for javadoc formatting
    checkstyle {
      // Point to the same checkstyle.xml file as the checkstyle task
      configFile file("$rootDir/checkstyle.xml")
    }
  }
}
# checkstyle.xml
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="InvalidJavadocPosition"/>
    <module name="JavadocBlockTagLocation"/>
    <module name="JavadocContentLocation"/>
    <module name="JavadocMethod"/>
    <module name="JavadocVariable"/>
    <module name="JavadocStyle"/>
    <module name="JavadocTagContinuationIndentation"/>
    <module name="MissingJavadocType"/>
    </module>
</module>

Javadoc formatting options are available here: https://checkstyle.sourceforge.io/checks/javadoc/index.html

@at88mph
Copy link
Member Author

at88mph commented Jan 8, 2025

Most of these changes look good, except we need significant documentation, since we are touching so many files.

To enable docstrings checks, following edits are needed

// build.gradle
spotless {
  java {
    // Interpret all files as utf-8
    encoding 'UTF-8'
    // Only require formatting of files that diff from main
    ratchetFrom 'origin/main'
    // Use the default importOrder configuration
    importOrder()
    // Remove unused imports
    removeUnusedImports()
    // Google Java Format, Android Open Source Project style which uses 4 spaces for indentation
    palantirJavaFormat('2.50.0').formatJavadoc(true)
    // Format annotations on a single line
    formatAnnotations()
    // Checks for javadoc formatting
    checkstyle {
      // Point to the same checkstyle.xml file as the checkstyle task
      configFile file("$rootDir/checkstyle.xml")
    }
  }
}
# checkstyle.xml
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="InvalidJavadocPosition"/>
    <module name="JavadocBlockTagLocation"/>
    <module name="JavadocContentLocation"/>
    <module name="JavadocMethod"/>
    <module name="JavadocVariable"/>
    <module name="JavadocStyle"/>
    <module name="JavadocTagContinuationIndentation"/>
    <module name="MissingJavadocType"/>
    </module>
</module>

Javadoc formatting options are available here: https://checkstyle.sourceforge.io/checks/javadoc/index.html

That's done, thanks.

@at88mph at88mph merged commit 5ee0043 into opencadc:main Jan 8, 2025
9 checks passed
@at88mph at88mph deleted the kueue-feature-flag branch January 8, 2025 23:54
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