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

Start containers at the end of deployment #1925

Closed
wants to merge 6 commits into from

Conversation

avitomar12
Copy link

This PR resolve the issue #1840 . Added code to start the container.

@google-cla google-cla bot added the cla: yes label Aug 20, 2021
Copy link
Contributor

@berggren berggren left a comment

Choose a reason for hiding this comment

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

Thanks for contributing. Please take a look at my comment and let me know what you think.

echo "2. docker-compose up -d"
docker-compose up -d
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure everyone wants to automatically start the containers. Could we put this behind a flag instead?

--start-containers

Copy link
Author

Choose a reason for hiding this comment

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

Where do input this flag? Can we ask user whether to start container after installation is done?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think both would be fine:
sh deploy_timesketch.sh —start-container

Or if that is not set at the end of the script ask
Would you like start your container (y/n) [default:no]

What do you think @avitomar12 ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. I also think its better to provide both options to the user.

echo "3. docker-compose exec timesketch-web tsctl add_user --username <USERNAME>"
#input the username
read -p 'USERNAME : ' USRNAME
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should that be s/USRNAME/USERNAME?

Copy link
Collaborator

@jaegeral jaegeral left a comment

Choose a reason for hiding this comment

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

Some comments inline

@berggren
Copy link
Contributor

Friendly ping @avitomar12 - please let us know the status of this PR and if you plan to update it.

@berggren berggren changed the title Added deploy script to start containers at the end of run Start containers at the end of deployment Nov 24, 2021
@jaegeral
Copy link
Collaborator

@avitomar12 Friendly ping

@berggren
Copy link
Contributor

berggren commented Feb 6, 2022

@avitomar12 Ping again - please see the comments in the review. If we don't hear back we need to close this PR for the time being (we can open it again if needed later).

@avitomar12
Copy link
Author

@avitomar12 Ping again - please see the comments in the review. If we don't hear back we need to close this PR for the time being (we can open it again if needed later).

@berggren I am working on it. I will make change within two days.

@berggren
Copy link
Contributor

berggren commented Mar 8, 2022

@avitomar12 Ping again - please see the comments in the review. If we don't hear back we need to close this PR for the time being (we can open it again if needed later).

@berggren I am working on it. I will make change within two days.

Hello again, any progress on this PR?

@berggren
Copy link
Contributor

I'm closing this PR out of lack of updates. Please reopen if you get a chance to pick this up again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants