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

Some small fixes here and there #661

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Some small fixes here and there #661

wants to merge 4 commits into from

Conversation

Itxaka
Copy link
Member

@Itxaka Itxaka commented Jan 14, 2025

Including:

  • init empty vars as the type
  • fix import order
  • fix error strings
  • fix unused imports
  • remove unused functions (if needed by anything, they shoudl go into sdk)
  • fix functions comments
  • improve constants

@Itxaka Itxaka requested a review from a team January 14, 2025 10:04
@@ -172,7 +172,9 @@ func sharedReset(reboot, unattended, resetOem bool, dir ...string) (c *config.Co

d, err := json.Marshal(r)
if err != nil {
c.Logger.Errorf("failed to marshal reset cmdline flags/event options: %s", err)
if &c != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

this was a potential nil reference

@@ -442,7 +424,7 @@ func scan(result *Config, opts ...collector.Option) (c *Config, err error) {
}
}

if !kc.IsValid() {
if kc != nil && !kc.IsValid() {
Copy link
Member Author

Choose a reason for hiding this comment

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

potential nil reference

Copy link
Contributor

@jimmykarily jimmykarily left a comment

Choose a reason for hiding this comment

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

Only a couple of comments. Looks good.

@@ -339,20 +335,6 @@ type Bundle struct {
Targets []string `yaml:"targets,omitempty"`
}

const DefaultHeader = "#cloud-config"

func HasHeader(userdata, head string) (bool, string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

mmmh should be implemented there on in the sdk, makes no sense to have a function here that its used bu another library lool

@@ -29,7 +29,7 @@ type OCIImageExtractor struct{}

var _ ImageExtractor = OCIImageExtractor{}

func (e OCIImageExtractor) ExtractImage(imageRef, destination, platformRef string) error {
func (e OCIImageExtractor) ExtractImage(imageRef, destination, _ string) error {
img, err := utils.GetImage(imageRef, utils.GetCurrentPlatform(), nil, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we be doing some check that the platformRef matches the current platform or something like that? You already did something like that in the kairos-sdk: https://github.com/kairos-io/kairos-sdk/pull/557/files#diff-5a53c19741f79228c7414fd4f325e7d79935eb2b5c051ae2e44c0d9521542006R111

or is that not the same case?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC we are using the sdk here as a backend, this methods were here becuase it was part of the agent before we put in on the sdk and then we moved but could not update or something so we leaved this stub in here. we should be using the sdk ones directly!

Itxaka and others added 3 commits February 13, 2025 16:57
Including:
 - init empty vars as the type
 - fix import order
 - fix error strings
 - fix unused imports
 - remove unused functions (if needed by anything, they shoudl go into
   sdk)
 - fix functions comments
 - improve constants

Signed-off-by: Itxaka <[email protected]>
Signed-off-by: Itxaka <[email protected]>
Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Copy link
Contributor

I fixed the failing test and rebased on main (fixed conflicts).

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 26.31579% with 28 lines in your changes missing coverage. Please review.

Project coverage is 48.41%. Comparing base (ae7b2d0) to head (f5e0ffd).

Files with missing lines Patch % Lines
internal/agent/hooks/mounts.go 0.00% 13 Missing ⚠️
internal/agent/interactive_install.go 0.00% 4 Missing ⚠️
internal/agent/reset.go 0.00% 3 Missing ⚠️
pkg/utils/common.go 50.00% 2 Missing ⚠️
internal/agent/hooks/kcrypt_uki.go 0.00% 1 Missing ⚠️
internal/agent/upgrade.go 0.00% 1 Missing ⚠️
pkg/config/config.go 50.00% 1 Missing ⚠️
pkg/config/spec.go 50.00% 1 Missing ⚠️
pkg/github/releases.go 0.00% 1 Missing ⚠️
pkg/types/v1/image.go 0.00% 1 Missing ⚠️

❌ Your project check has failed because the head coverage (6.63%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
+ Coverage   48.09%   48.41%   +0.31%     
==========================================
  Files          48       48              
  Lines        6161     6135      -26     
==========================================
+ Hits         2963     2970       +7     
+ Misses       2916     2885      -31     
+ Partials      282      280       -2     

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

@Itxaka
Copy link
Member Author

Itxaka commented Feb 13, 2025

I fixed the failing test and rebased on main (fixed conflicts).

still failing looooool

Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Copy link
Contributor

I fixed the failing test and rebased on main (fixed conflicts).

still failing looooool

not anymore :p

Comment on lines +128 to +131
_ = 1 << (10 * iota)
_
_
GiB
Copy link
Member

Choose a reason for hiding this comment

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

WAT? hehe I guess it just means we are not using the smaller units 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

xDD

I feel weird about that. Its some kind of magic and then it turns into real GiB....

@Itxaka
Copy link
Member Author

Itxaka commented Feb 14, 2025

I fixed the failing test and rebased on main (fixed conflicts).

still failing looooool

not anymore :p

conflicts looooool

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.

3 participants