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

feat: Add hidden snapshot command for future implementation #120

Merged
merged 17 commits into from
Apr 16, 2019

Conversation

maprules1000
Copy link
Contributor

Description

This PR adds a hidden snapshot command, and a stubbed static snapshot service that will be fleshed out in a future PR.

This also includes tests for the snapshot command.

@maprules1000 maprules1000 changed the title Snapshot command feat: snapshot command Apr 3, 2019
@maprules1000 maprules1000 changed the title feat: snapshot command feat: Add snapshot command Apr 3, 2019
@maprules1000 maprules1000 force-pushed the map/static-site-support branch from d024767 to 7987a09 Compare April 10, 2019 19:23
@maprules1000 maprules1000 force-pushed the map/static-site-support branch from 7987a09 to 73aa318 Compare April 10, 2019 19:33
@maprules1000 maprules1000 force-pushed the map/static-site-support branch from 73aa318 to 5de524c Compare April 10, 2019 19:34
@maprules1000 maprules1000 requested review from djones and Robdel12 April 10, 2019 19:39
@maprules1000
Copy link
Contributor Author

This PR is a stepping stone to #132 which will implement and test the underlying static snapshot service

Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

Looking really good! 😃 I cannot wait for this feature, it's going to be super rad

src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
import logger from '../utils/logger'
import {StaticSnapshotOptions} from './static-snapshot-options'

export default class StaticSnapshotService {
Copy link
Contributor

Choose a reason for hiding this comment

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

As I read through this, I wonder if it would make sense to have the snapshot service extend the agent service? Or at least take care of the setup of the agent service in this constructor, so you don't have to do that in the run command method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I feel like this conversation/decision might be better had on #137 since that fleshes out (and changes) the static snapshot service @Robdel12

src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
@maprules1000 maprules1000 changed the title feat: Add snapshot command feat: Add hidden snapshot command for future implementation Apr 12, 2019
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
export default class Constants {
static readonly PORT: number = 5338
static readonly NETWORK_IDLE_TIMEOUT: number = 50 // in milliseconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Fab

src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
src/commands/snapshot.ts Outdated Show resolved Hide resolved
@maprules1000 maprules1000 force-pushed the map/static-site-support branch from 8181cf7 to 07d3e5a Compare April 16, 2019 16:04
@maprules1000 maprules1000 requested a review from djones April 16, 2019 16:52
src/commands/snapshot.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@djones djones left a comment

Choose a reason for hiding this comment

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

🍍 LGTM

@maprules1000 maprules1000 merged commit 98ae4b8 into master Apr 16, 2019
@maprules1000 maprules1000 deleted the map/static-site-support branch April 16, 2019 19:10
djones pushed a commit that referenced this pull request Apr 16, 2019
# [0.3.0](v0.2.3...v0.3.0) (2019-04-16)

### Features

* Add hidden snapshot command for future implementation ([#120](#120)) ([98ae4b8](98ae4b8))
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.

4 participants