-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(cli)!: find bots by default paths #121
Conversation
@fubuloubu We should probably inform the developers that each file in the bots/ folder should be independent of one another. And that each file should be its own bot. I can add a long note in the README about this |
You can probably raise a warning if you see But docs definitely encouraged for the best way to package a module relative to the |
c45f054
to
0f37ee5
Compare
NOTE: marking breaking just due to the docs updates needed, and the changing expectations on how to write an app (which should get more widely noted w/ Silverback v0.6.0 alongside full cluster beta access work) |
1449733
to
bf76520
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion, looks good to me!
Needs some local testing from others (unsure if anyone else has checked it out yet)
I was able to run it with a simple update shareprice
result:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think that the silverback build
command should be producing an image if it can, by wrapping docker
(if available) in a particular "expected" way to build/tag the images e.g.
$ silverback build --generate
Generating './images/Dockerfile.botA'
Generating './images/Dockerfile.botB'
Generating './images/Dockerfile.botC'
Building './images/Dockerfile.botA' as 'github-org/repo-name-botA:latest'
ERROR: Command 'docker' not found, cannot build image
This would build with the tag github-org/repo-botA:latest
(if git is available, and you can find a github remote)
...but this might be getting into stuff that is best left to the github action to configure (basically make the GH action call this command and just keep this command simple with some sensible defaults)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do this, I really don't want to add another dependency to silverback though, so I'd use system level code to interact with the docker.sock. We don't normally do that though, so up to you. Like you said, might be getting into stuff that would be best to leave to the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I think in that case, silverback build
might not be the right name for the command (originally I was thinking silverback build --generate
or some variant to show that it was creating docker files first)
I wouldn't really call it a dependency though, it's either docker
command exists and we subprocess out to it in a specific way, or it doesn't and you raise an error and exit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember, we are just handling suggested behaviors here to make it easy for folks. Anything more complicated should be done manually, the silverback
subcommand only works if you follow those suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the silverback buld
terminology. I can add the option for --generate if you'd like. I really think we should get this out, then I can make a PR following this one to really dive into the build functionality to add the remainder of what we want here. Current functionality allows me to do everything we want to in the workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the
silverback buld
terminology. I can add the option for --generate if you'd like. I really think we should get this out, then I can make a PR following this one to really dive into the build functionality to add the remainder of what we want here. Current functionality allows me to do everything we want to in the workflow
The ideal workflow here is the average person doesn't have to touch docker
command at all if they follow our suggestions and have docker installed
This will be what our github action will do e.g. run: silverback build --generate
(which makes it easy to debug the github action if it is not working, just run same command locally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to be clear, we want silverback build
to run docker build
for each file in the .silverback-images
directory. And then we want silverback build --generate
to create the dockerfiles in the .silverback-images
directory. Correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to be clear, we want
silverback build
to rundocker build
for each file in the.silverback-images
directory. And then we wantsilverback build --generate
to create the dockerfiles in the.silverback-images
directory. Correct?
Yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can separate these out? It seems very likely that all users will need to manually fuck with their dockerfiles. Adding dependencies or data files. So maybe this should be two steps.
- Generate dockerfile templates for the known bot sources
- Build images for said bots
Or maybe add a --build
option that specifically runs docker build
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fubuloubu let me know if you want to add a build
flag. I think the current workflow is good, but I'd like your input here
bdca189
to
f9fd334
Compare
9f34789
to
a3aaeb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it!
What I did
In an attempt to make the dev ux simpler, I allow for the path to be an option. If the path does not exist, we set the bath to
pathlib.Path.cwd()/bots/bot.py
We check and raise if the file and/or the directory does not exist.
We are also adding a
Dockerfile
generator to the_cli.py
file that will search thebots/
directory for filenames. Each filename will be generated as aDockerfile.<filename>
. The Dockerfile will copy the filename into thebots/
directory asbot.py
.fixes: #122
fixes: #123
fixes: SBK-564
fixes: SBK-563
How I did it
Made minor changes to the
run
command and adding a method to generate the dockerfiles.How to verify it
Without a
bots/
directory in the root of the project run:This will raise and show a:
Then:
This will raise and show:
You can then run:
And you will get an error that does not relate to files missing.
You can also run:
And you will get an error that does not pertain to the files/directory missing.
MORE TO BE ADDED
Checklist