eve icon indicating copy to clipboard operation
eve copied to clipboard

[WIP] refactor: replace hardcoded paths with constants from locationconsts.go (#3682)

Open jeff-zed opened this issue 6 months ago • 10 comments

This PR resolves Issue #3682 by replacing hardcoded filesystem paths with centralized constants from locationconsts.go across the pillar modules, improving code maintainability and consistency. The refactoring centralizes path management while preserving existing functionality and maintaining clear separation between EVE-managed paths and system paths.

Currently, 5 modules have been updated pending test verification (see commits for details):

  • pkg/pillar/types/locationconsts.go

  • pkg/pillar/agentlog/*

  • pkg/pillar/base/*

  • pkg/pillar/hypervisor/*

  • pkg/pillar/pidfile/*

PR dependencies

How to test and validate this PR

make test pkg/pillar

Changelog notes

This addresses technical debt by centralizing filesystem path management, making future path changes easier to implement and reducing the risk of inconsistent path usage across the codebase.

PR Backports

N/A - This is a refactoring change that improves maintainability without changing functionality.

Checklist

  • [x] I've provided a proper description
  • [x] I've added the proper documentation (when applicable)
  • [ ] I've tested my PR on amd64 device(s)
  • [ ] I've tested my PR on arm64 device(s)
  • [x] I've written the test verification instructions
  • [x] I've set the proper labels to this PR

jeff-zed avatar Jun 12 '25 10:06 jeff-zed

@eriknordmark this will be a multi-commit PR, module by module.

jeff-zed avatar Jun 12 '25 11:06 jeff-zed

@milan-zededa @christoph-zededa

There is a minor opportunity to centralize the path "/run/eve-hv-type"

https://github.com/lf-edge/eve/blob/7ab0c0b8b14650228e9ff3508a2606ba889fe8ab/pkg/pillar/base/kubevirt.go#L15-L16

https://github.com/lf-edge/eve/blob/7ab0c0b8b14650228e9ff3508a2606ba889fe8ab/pkg/pillar/hypervisor/hypervisor.go#L98

However, pkg/pillar/types/dns.go imports pkg/pillar/base for pkg/pillar/base/logobjecttypes.go

https://github.com/lf-edge/eve/blob/7ab0c0b8b14650228e9ff3508a2606ba889fe8ab/pkg/pillar/types/dns.go#L15

Because pkg/pillar/base is used in so many modules (not limited to pkg/pillar/base/logobjecttypes.go), I am inclined to leave this as-is.

Alternatively, we could just remove this function and refactor its limited use:

https://github.com/lf-edge/eve/blob/7ab0c0b8b14650228e9ff3508a2606ba889fe8ab/pkg/pillar/hypervisor/hypervisor.go#L96C1-L99C2

jeff-zed avatar Jun 13 '25 13:06 jeff-zed

@jeff-zed I'm not sure if I follow the issue with the pillar/base package. Are you getting import cycle or some other build issue if you change BootTimeHypervisor to:

func BootTimeHypervisor() Hypervisor {
	return bootTimeHypervisorWithHVFilePath(base.EveVirtTypeFile)
}

? Or were you considering to move EveVirtTypeFile to pillar/types?

milan-zededa avatar Jun 13 '25 13:06 milan-zededa

@jeff-zed I'm not sure if I follow the issue with the pillar/base package. Are you getting import cycle or some other build issue if you change BootTimeHypervisor to:

func BootTimeHypervisor() Hypervisor {
	return bootTimeHypervisorWithHVFilePath(base.EveVirtTypeFile)
}

? Or were you considering to move EveVirtTypeFile to pillar/types?

cyclical dependency

jeff-zed avatar Jun 13 '25 13:06 jeff-zed

The runner deployment script seems to be malfunctioning. @rene is currently working on fixing it.

OhmSpectator avatar Jun 13 '25 14:06 OhmSpectator

@jeff-zed apart from the Yetus errors yet to be fixed, it looks good to me...

There is still some more to commit. When I think I am done and it is ready for full review, I will add a comment indicating that for the reviewers.

jeff-zed avatar Jun 17 '25 07:06 jeff-zed

@jeff-zed apart from the Yetus errors yet to be fixed, it looks good to me...

There is still some more to commit. When I think I am done and it is ready for full review, I will add a comment indicating that for the reviewers.

Until a PR is ready for review, we usually mark it as Draft and add [WIP] to the title.

OhmSpectator avatar Jun 17 '25 07:06 OhmSpectator

@jeff-zed apart from the Yetus errors yet to be fixed, it looks good to me...

There is still some more to commit. When I think I am done and it is ready for full review, I will add a comment indicating that for the reviewers.

Until a PR is ready for review, we usually mark it as Draft and add [WIP] to the title.

Thank you.

jeff-zed avatar Jun 17 '25 07:06 jeff-zed

The runner deployment script seems to be malfunctioning. @rene is currently working on fixing it.

Thank you. This should also be helpful for EVE 15.3.0 that @eriknordmark tagged

jeff-zed avatar Jun 17 '25 08:06 jeff-zed

Hi all. I noticed that pkg-deps.mk is currently ignored via .gitignore, but it needs to be versioned so CI picks up our updated dependency list.

Would you prefer I remove the *.mk (or pkg-deps.mk) entry from .gitignore, or simply force-add this one file?

There is also the question of whether edgeview should be modified to use the pkg/pillar/types/locationconst.go or not in the latest commit.

Thanks!

jeff-zed avatar Jun 17 '25 10:06 jeff-zed