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

Service arbitrary commands #15

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sarahloui
Copy link
Collaborator

No description provided.

@sarahloui sarahloui force-pushed the service_arbitrary_commands branch from 92c80f7 to f7e4e37 Compare September 13, 2023 10:16
lib/cove/cli/service.rb Outdated Show resolved Hide resolved
end

def name
"#{service.name}-#{role.name}-#{version}"
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use the SecureRandom class to create a random string and attach it to the container to keep the name unique.

I would also suggest adding “run” or something to the name to better visually distinguish it from the deployed containers.

Copy link
Contributor

@jfahrer jfahrer left a comment

Choose a reason for hiding this comment

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

This is looking good. Regarding the labels we talked about on how to identify on-demand instances VS deployed instances.

I think a Label cove.type with on-demand or deployed as the values sound like the right path forward. You could apply this label in the respective instance classes. And then change the code that finds existing containers to also respect that label. Feel free to do this in a separate PR.

@@ -7,6 +7,10 @@ def self.start_container(*containers)
[:docker, "container", "start", *containers.flatten]
end

def self.start_attached_container(container)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def self.start_attached_container(container)
def self.start_container_and_attach(container)

Feels more descriptive for what is going on.

# @param [Cove::DesiredContainer] config
# @return [Array] The command to create the container
def self.run_container(config)
Docker::Container::Run.build(image: config.image, name: config.name, remove: true, detach: false, interactive: true, labels: config.labels, command: config.command, ports: config.ports, mounts: config.mounts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this and related code since it is not used?

@@ -0,0 +1,39 @@
module Cove
class InstanceOnDemand
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you feel naming this OnDemandInstance. Another approach could be to make an Instance module and have Instance::OnDemand and Instance::Deployed or something. That would keep those two very closely related concepts closer together.

@index = 1
end

def name
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the doc for the return type as well? Thanks.

end

def labels
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be deleted, otherwise it overwrites the delegate for labels above.

@sarahloui sarahloui force-pushed the service_arbitrary_commands branch from c8d83b6 to b5d1c7b Compare January 13, 2024 00:50
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.

2 participants