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

add service list to gui #26

Merged
merged 15 commits into from
Feb 13, 2023
Merged

add service list to gui #26

merged 15 commits into from
Feb 13, 2023

Conversation

medyagh
Copy link
Contributor

@medyagh medyagh commented Feb 10, 2023

can be merged after #24

Screenshot 2023-02-10 at 11 11 19 AM

  • adding tunnell headers and skaffolding
  • add commadn runner for tunnel
  • add tunnel implemation
  • add service button
  • add signal

the gui will need to use json instead of table when this is done kubernetes/minikube#15827

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 10, 2023
Copy link
Member

@spowelljr spowelljr left a comment

Choose a reason for hiding this comment

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

I don't see any service logic, I just see the button being created but not the call to minikube.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 13, 2023
@medyagh
Copy link
Contributor Author

medyagh commented Feb 13, 2023

I don't see any service logic, I just see the button being created but not the call to minikube.

Good catch I totally forgot to commit those files, now it should be good

src/operator.h Outdated Show resolved Hide resolved
src/serviceView.h Outdated Show resolved Hide resolved
src/serviceView.cpp Outdated Show resolved Hide resolved
src/serviceView.cpp Outdated Show resolved Hide resolved
src/commandrunner.cpp Outdated Show resolved Hide resolved
minikube.pro Outdated Show resolved Hide resolved
void ServiceView::displayTable(QString svcCmdOutput)
{

m_dialog = new QDialog(m_parent);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
m_dialog = new QDialog(m_parent);
QDialog dialog = new QDialog(m_parent);

This doesn't need to be a class wide variable as the variable isn't used outside this function.

src/basicview.cpp Outdated Show resolved Hide resolved
src/serviceView.h Outdated Show resolved Hide resolved
@medyagh
Copy link
Contributor Author

medyagh commented Feb 13, 2023

ready for another look

dockerEnvButton->setEnabled(isRunning || isPaused);
sshButton->setEnabled(isRunning || isPaused);
mountButton->setEnabled(isRunning || isPaused);
tunnelButton->setEnabled(isRunning || isPaused);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't have to be done in this PR, but we should make a variable called isRunningOrPaused instead of repeatedly doing isRunning || isPaused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea

Comment on lines +38 to +41
QLabel *tableLbl = new QLabel();
tableLbl->setOpenExternalLinks(true);
tableLbl->setWordWrap(true);
tableLbl->setText(svcCmdOutput);
Copy link
Member

Choose a reason for hiding this comment

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

Not required, but going forward you can pass a QString to the QLabel constructor and it will pass the QString to setText in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good to know thanks

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: medyagh, spowelljr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@medyagh medyagh merged commit af54fee into main Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants