Conversation
…'app stop' working concepts
…dy up command logic, implemented stop and start app commands
…ror output on validation fail
…ommand (bug detected)
inverse
left a comment
There was a problem hiding this comment.
Didn't try it yet and i've not used Golang for a while but did a spot check and it looks good :) awesome work for keeping going 💪🏼
| func taskSetReleaseVersion() string { | ||
|
|
||
| fmt.Println("Executing taskSetReleaseVersion") | ||
| // fmt.Println("Executing taskSetReleaseVersion") |
There was a problem hiding this comment.
Why are these all commented out? do we need to have like a flag for verbosity?
There was a problem hiding this comment.
Exactly 😉
These are nice to have because they are logged as well when running edgeboxctl as a service. But for the CLI it does not make so much sense (as we want either the desired output or an error message).
Still need to add this step (verbosity control), which would work something like this:
- System Service verbosity is on by default at all times
- CLI commands have no verbosity by default
- An boolean property in /home/$USER/Edgebox/config.yml can be set to override the default CLI behaviour (cli-verbosity: true)
- additionally, a command line flag (--verbose or -v) can be used to force a CLI command to print everything as well, in a command by command basis
| func GetBaseTask(taskCode string, argsString string) Task { | ||
| currentFormattedTime := utils.GetFormattedDateTime(time.Now()) | ||
| task := Task{ | ||
| ID: 0, |
There was a problem hiding this comment.
Where is this incremented?
There was a problem hiding this comment.
This ID is the primary key for the Tasks database table (the postgresSQL DB of Symfony).
Each task issued by the API will have an unique ID so it can be identified, updated, and loaded in the database.
When running as a service, edgeboxctl picks the newest non executing task from this table.
In the case of running a CLI command, this ID is always 0 for now, but would be nice to also add those as new entries in the database using insert (as this way commands executed in the CLI would also appear in the dashboard! 🤩).
This is something that needs to be figured out yet. this is still a bit "shady" since edgeboxctl reads and writes to the symfony database effectively coupling the two projects, and the objective of this PR is that you can run all Edgebox features from the CLI without depending on the API part. I see some solutions here like implementing something more generic that could be expanded to work with any data store or even use multiple data stores (a postgresSQL Database can be one, Redis other, etc). This would allow more control and data could be accessed by both projects (edgrboxctl and API) to exchange data. Additionally we can have clear logic to deal with loading a data store to be used by edgeboxctl (can also be a static config), and dealing with things like a data store not being available ( so it gracefully detects it and logs it ).
What do you think of the idea? I think this can be dealt with in another PR (to make it more resilient and expandable), but we could at least for now try:
- loading the DB but not crash if the DB is not available
- if a task is issued via the CLI, try inserting it as a new task in the DB (so it gets an auto incremented ID)
There was a problem hiding this comment.
How does this work though? So like we generate this task and tries to update the task with ID 0 in the sqlite DB?
We're basicly usign the DB as a message bus though right? like either the CLI or API pushing tasks to be executed by the CLI... There is no other coupling aside this tasks table? I don't think that's so bad... or am I missing something?
There was a problem hiding this comment.
How does this work though? So like we generate this task and tries to update the task with ID 0 in the sqlite DB?
If you are executing commands from the CLI, yes.
(...) like either the CLI or API pushing tasks to be executed by the CLI...
Yes. that's the gist of it. But a task executed in the CLI does not go to the task queue. Only when running as a service edgeboxctl uses the DB to load the next task and execute, then updating the DB entry with the result json.
When you execute a task via the CLI command, it creates a "Task" with ID=0, which at some point edgeboxctl will try to run the following SQL query: "UPDATE tasks SET result="[something] WHERE ID=0", but since there is not entry with this ID, it will fail silently and not register anything. What can be done here, is: when getting the base task, we can insert a new entry in the tasks db (would have to add logic to not execute that entry in the service). This will result in the hability to log commands ran from the CLI and making them appear in the dashboard. I consider this secondary at this point though :)
There is no other coupling aside this tasks table?
There is also the Options table, which is used as a key/value pair store of sorts. But yeah these are the only coupling things to the API atm. Also easy enough to decouple / make it resilient if it cannot load a DB.
Yeah in general that is not so bad :) I think we don't need to worry too much about it for now. As long as the program is resilient and does not crash when trying to write/read from a DB that possibly does not exist.
There was a problem hiding this comment.
Could we not get this function to insert the "baseTask" and like use the auto-increment ID of the DB? Or have the caller of this function be like
func getCommandTask(taskSlug string, taskArgs string, execute bool) tasks.Task {
task := tasks.GetBaseTask(
taskSlug,
taskArgs,
)
task = tasks.InsertTask(task)
if execute {
task = tasks.ExecuteTask(task)
}
return task
}Co-authored-by: Malachi Soord <inverse.chi@gmail.com>
Thanks! 🚀 Still some things to go, but I'm liking a lot where this is going 💪 I'll add some further comments on this PR with what's coming next and the reasoning behind it. |
| } | ||
|
|
||
| log.Printf("Starting edgeboxctl service for %s", *name) | ||
| func checkArgumentsPresence(c *cli.Context, count int) error { |
There was a problem hiding this comment.
Crazy that there is no way to validate positional arguments with this library :/
I saw https://github.com/urfave/cli/blob/master/docs/v2/manual.md#required-flags
But that's just for flags
There was a problem hiding this comment.
Yeah, I still think that maybe I'm missing something? 😆
Anyway, this workaround should suffice for validating the presence of n number of arguments expected.
Adding proper support for CLI handling via external library. Objective is to be able to run single actions of the edgebox system through CLI commands.
Some examples of this:
edgeboxctl tunnel setupedgeboxctl app start --alledgeboxctl backup setupedgeboxctl backup statusedgeboxctl service start...All implemented edgebox commands that are able to be run as single actions should be implemented as CLI commands.
The CLI commands aim to provide a true independent usable experience around the edgebox system (without depending on the API/Dashboard), and facilitate setup through centralizing system
setup and handling logic within this project.
This step represents an effort to build a system that can be easily installed and managed on any device, to allow an independent usage of the following feature set: