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

Toggle Polling from the UI #174

Merged

Conversation

Skarlso
Copy link
Member

@Skarlso Skarlso commented Feb 23, 2019

No description provided.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 23, 2019

@michelvocks I might have to pick your brain on how to go about with a Toggle button. I want to get its state and set it to green or something based on the state of the config. And then toggling that will change the config value.

@codecov-io
Copy link

codecov-io commented Feb 23, 2019

Codecov Report

Merging #174 into master will decrease coverage by 0.12%.
The diff coverage is 63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
- Coverage   66.82%   66.69%   -0.13%     
==========================================
  Files          33       35       +2     
  Lines        2607     2681      +74     
==========================================
+ Hits         1742     1788      +46     
- Misses        642      671      +29     
+ Partials      223      222       -1
Impacted Files Coverage Δ
store/settings.go 0% <0%> (ø)
handlers/handler.go 85.96% <100%> (+0.77%) ⬆️
store/store.go 67.56% <50%> (-1.01%) ⬇️
handlers/settings.go 70.21% <70.21%> (ø)
workers/pipeline/ticker.go 50% <89.28%> (+13.49%) ⬆️
workers/pipeline/git.go 69.33% <0%> (-2.8%) ⬇️
handlers/pipeline.go 53.94% <0%> (-2.31%) ⬇️
workers/pipeline/create_pipeline.go 44.08% <0%> (-1.47%) ⬇️
handlers/hook.go 62.5% <0%> (-0.66%) ⬇️
handlers/pipeline_run.go 0% <0%> (ø) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 519a2be...131b37d. Read the comment docs.

@michelvocks
Copy link
Member

@michelvocks I might have to pick your brain on how to go about with a Toggle button. I want to get its state and set it to green or something based on the state of the config. And then toggling that will change the config value.

You can use vue-bulma-switch for this. It's pretty old and not really updated but basically all our current components are based on vue-bulma so I think it makes sense.

Let me know if you need something else.

workers/pipeline/ticker.go Outdated Show resolved Hide resolved
@Skarlso
Copy link
Member Author

Skarlso commented Feb 23, 2019

@michelvocks I might have to pick your brain on how to go about with a Toggle button. I want to get its state and set it to green or something based on the state of the config. And then toggling that will change the config value.

You can use vue-bulma-switch for this. It's pretty old and not really updated but basically all our current components are based on vue-bulma so I think it makes sense.

Let me know if you need something else.

Cool, thanks!

@Skarlso
Copy link
Member Author

Skarlso commented Feb 24, 2019

Uh, we will have to eventually take this apart into smaller files or something. Navigating this view is becoming difficult. :) I don't know how vue works but that should be possible. :)

@Skarlso
Copy link
Member Author

Skarlso commented Feb 24, 2019

Oh! I see how permissions did it. I'll do that same. :) Extract settings into there. Nice.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 24, 2019

@michelvocks This looks like shit right now. :D But actually works. :) the toggle and all work together stopping / starting the poller. :) And the button is set to on and off on mounting so it shows what the value currently is. Which is nice. :D
screenshot 2019-02-24 at 21 40 15
screenshot 2019-02-24 at 21 43 04

I also tried turning it on and off a hundred times or so quickly to see if I can break the underlying channel logic. But seems to work okay. :)

I'll try and come up with a design for this. And write some handler tests. :)

@michelvocks
Copy link
Member

@Skarlso Cool. Looking forward to trying it out. This is also a solid foundation for additional settings 👍

@Skarlso
Copy link
Member Author

Skarlso commented Feb 25, 2019

@michelvocks yeah I'm trying to work out a better way of doing this 🤔 Maybe save all the possible settings in a map pointer with string keys or something. I'll do some more thinking on it. It would be cool to have something common that all the settingon-off methods could use.

You still would have get set methods for individual settings but they would use something common. But maybe it's not worth it. Don't know until I have a go at it. :) I don't want to use reflection either.

@michelvocks
Copy link
Member

@Skarlso
Copy link
Member Author

Skarlso commented Feb 25, 2019

Yes. But the settings are of different types. And I sort of thought for something like: setSetting(name, key, string) bamm, done. :D But that's not going to work. Unless we use something like, flag.Func or something like that which takes a function and would handle conversion. Not sure yet.

@michelvocks
Copy link
Member

I think it's easier to send all settings in a struct and check in the backend what has changed.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 25, 2019

I was thinking about that too to just always send everything. But I don't like that. :) I would like to keep the Json size to a bare minimum. If we always send / get everything that will increase the amount of bandwidth Gaia is using.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 25, 2019

Eh, I don't know. Maybe it's okay for now. I don't see how we'd change some of the other config settings like port of server address or working directory without the need to restart Gaia anyways. Maybe if another setting presents itself, then we'll consider refactoring.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 26, 2019

@michelvocks found a nicer button ;)
screenshot 2019-02-26 at 21 47 37
screenshot 2019-02-26 at 21 47 32

Easier to change programatically too.

@michelvocks
Copy link
Member

Nice! 🤗

@Skarlso Skarlso changed the title [WIP] Toggle Polling from the UI Toggle Polling from the UI Feb 27, 2019
@Skarlso
Copy link
Member Author

Skarlso commented Feb 27, 2019

@michelvocks Read for you man. :) Increases coverage \o/

Copy link
Member

@michelvocks michelvocks left a comment

Choose a reason for hiding this comment

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

One minor thing. Overall LGTM 👍

@michelvocks
Copy link
Member

@Skarlso One additional thing we should think about:

What happens when you restart Gaia or it crashes? The change you made in the UI is basically lost.
This might be frustrating for users. We could save the change in the boltdb but then we have to decide which change takes precedence in case the user set an environment variable or used a configuration file.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 28, 2019

@michelvocks Our flag implementation now supports environment files. We could write this out in that file which does take precedence.

I'm reluctant to use a dB to configure start up time configuration. The Environment file is a better choice in that case I think.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 28, 2019

Uh, the file approach sucks because I can't really overwrite. I can search and append, or fseek. But it isn't particularly pretty. :/

I will have to resort to the DB.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 28, 2019

Oh yeah so the question is, which one would be used first.... I'm going with the bbolt setting being of higher importance.

frontend/client/views/settings/index.vue Show resolved Hide resolved
handlers/settings.go Outdated Show resolved Hide resolved
handlers/settings.go Outdated Show resolved Hide resolved
store/pipeline.go Outdated Show resolved Hide resolved
handlers/settings.go Outdated Show resolved Hide resolved
handlers/settings.go Outdated Show resolved Hide resolved
@Skarlso
Copy link
Member Author

Skarlso commented Mar 1, 2019

@michelvocks alright. this is ready for review again. I tested it manually too. Turned it on, quit Gaia, started it up again, and setting was still on. :)

@Skarlso Skarlso force-pushed the enable_disable_polling_from_ui branch from 28becd5 to 131b37d Compare March 1, 2019 14:59
Copy link
Member

@michelvocks michelvocks left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot @Skarlso 🤗

@michelvocks michelvocks merged commit 56d9c03 into gaia-pipeline:master Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants