Skip to content

Conversation

vaspahomov
Copy link
Contributor

No description provided.

@lucab
Copy link
Contributor

lucab commented Dec 15, 2020

@vaspahomov thanks for your patch. I've left a few comments in review, and it would be good to have a test for this too.

@vaspahomov vaspahomov force-pushed the feature/add-GetUnitFileState-method branch 2 times, most recently from 586b8f6 to 3ef922d Compare December 15, 2020 17:02
@vaspahomov vaspahomov force-pushed the feature/add-GetUnitFileState-method branch from 3ef922d to db69fcc Compare December 15, 2020 17:02
Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

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

Code looks fine but CI is failing. I think this is because you are trying to re-use a system service unit that may not exist on older OS version. It would be good to try with a dedicated unit instead.

func TestGetUnitFileState(t *testing.T) {
conn := setupConn(t)
defer conn.Close()
service := "systemd-udevd.service"
Copy link
Contributor

Choose a reason for hiding this comment

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

This may not exist in the testing environment. It would be good to set up a dedicated temporary service unit instead.

@GaikwadPratik
Copy link

@vaspahomov ,

Can you please resolve conflicts so @lucab can merge it?

@sigmonsays
Copy link

looking to call this method (GetUnitFileState) but it's just stale. Is there another way?

return status, nil
}

// GetUnitFileStateContext returns UnitFileState
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. You don't have to add Context to the method name -- all new methods accept context.
  2. What is UnitFileState and where it is defined?


// GetUnitFileStateContext returns UnitFileState
func (c *Conn) GetUnitFileStateContext(ctx context.Context, name string) (string, error) {
var prop dbus.Variant
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you just use string here?

@kolyshkin
Copy link
Collaborator

@vaspahomov please let us know if you want to keep working on this (and I apologize for a late response).

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.

6 participants