Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

fix(snap): Refactor to avoid conflicts with readonly config provider directory #51

Merged
merged 3 commits into from
Mar 21, 2023

Conversation

MonicaisHer
Copy link
Contributor

@MonicaisHer MonicaisHer requested a review from farshidtz March 17, 2023 16:03
snap/local/helper-go/install.go Outdated Show resolved Hide resolved
@@ -19,6 +19,8 @@ import (
"os"
)

const app = "kuiperd"
Copy link
Member

Choose a reason for hiding this comment

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

Having a global variable called app is confusing because this snap has two apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been updated to const daemonApp = "kuiperd".

Copy link
Member

Choose a reason for hiding this comment

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

My comment was about the confusion in name when set as a global variable in main.go, not the local variable in configure(), but it is a good improvement in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so would you like me to keep it as a local variable?

@MonicaisHer
Copy link
Contributor Author

MonicaisHer commented Mar 20, 2023

  • Rerun the snap testing once this PR has been merged.

@MonicaisHer MonicaisHer requested a review from farshidtz March 20, 2023 09:40
snap/local/bin/source-env-file.sh Outdated Show resolved Hide resolved
snap/local/bin/source-env-file.sh Outdated Show resolved Hide resolved
Co-authored-by: Farshid Tavakolizadeh <[email protected]>
@MonicaisHer MonicaisHer changed the title fix(snap): Refactor to avoid conflicts with readonly config provider … fix(snap): Refactor to avoid conflicts with readonly config provider directory Mar 20, 2023
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks

@farshidtz farshidtz merged commit 9b5a24a into canonical:main Mar 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants