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 intermediate netrc struct and custom JSON marshaling methods to Netrc #20

Merged
merged 1 commit into from
Feb 24, 2016
Merged

Conversation

jackspirou
Copy link
Contributor

This PR attempts to fix a bug with the Netrc JSON struct tag being named "user" instead of "password" while maintaining compatibility with current plugins that rely on the "user" syntax.

You can find the problematic line here: https://github.com/drone/drone-go/blob/master/drone/types.go#L124

More information about .netrc the file format here: https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.aix.files/netrc.htm

@tboerger
Copy link
Contributor

LGTM

@jackspirou
Copy link
Contributor Author

You can play with the implementation at http://play.golang.org/p/nyjvgCsUBV

@jackspirou
Copy link
Contributor Author

After reading through the codebase I realized a JSON Unmarshal interface is needed as well. I have added it. You can play with an example here http://play.golang.org/p/-IkM7KTf8W.

Tested and working!

@jackspirou jackspirou changed the title Adding DeprecatedUser and custom marshaling method to Netrc Add DeprecatedUser and custom JSON Marshaling methods to Netrc Feb 13, 2016
@tboerger
Copy link
Contributor

Still LGTM

@jackspirou
Copy link
Contributor Author

@bradrydzewski and @gtaylor any thoughts?

@bradrydzewski
Copy link
Member

few thoughts ... first is to add some unit tests just to ensure we have all our edge cases covered. Also although slightly less efficient (it will allocate an intermediary struct) what if we simplify with something like this:

type netrc struct {
    Machine  string `json:"machine"`
    Login    string `json:"login"`
    Password string `json:"password"`
    PasswordOld string `json:"user"`
}

func (n *Netrc) UnmarshalJSON(b []byte) error {
  x := &netrc{}
  err := json.Unmarhsal(b, x)
  if err != nil {
    return 
  }
  n.Machine = x.Machine
  n.Login = x.Login
  n.Password = x.Password
  if x.PasswordOld != "" {
    n.Password = x.PasswordOld
  }
  return nil
}

func (n Netrc) MarshalJSON() ([]byte, error) {
  x := &netrc{
    Machine: x.Machine,
    Login: x.Login,
    Password: x.Password,
    PaswordOld: x.Password
  }
  return json.Marshal(x)
}

@jackspirou
Copy link
Contributor Author

@bradrydzewski I started with a similar approach, but then ran into known issues around creating an endless recursion calls with json.Marshal -> MarshalJSON -> json.Marshal -> MarshalJSON etc... This creates an overflow error:

runtime: goroutine stack exceeds 250000000-byte limit
fatal error: stack overflow

I made some updates to your example above and you can see the results at http://play.golang.org/p/rYFGu5jglg.

I can def add unit tests and change DeprecatedUser to PasswordOld if you prefer that name.

@bradrydzewski
Copy link
Member

@jackspirou we can have Netrc and netrc to avoid the loop. Check out http://play.golang.org/p/-SkUAiCk23

@jackspirou
Copy link
Contributor Author

@bradrydzewski absolutely. Sorry if that was what you meant by:

it will allocate an intermediary struct

and I was too thick go get it. 😞

@bradrydzewski
Copy link
Member

no worries! I have a bit of an unfair advantage because I spent about two weeks playing with custom yaml marshaling code and working around Go static typing limitations. It stretched my mind 😄

@jackspirou jackspirou changed the title Add DeprecatedUser and custom JSON Marshaling methods to Netrc Add intermediate netrc struct and custom JSON marshaling methods to Netrc Feb 18, 2016
…ON password attribute bug and maintain compatibility with current plugins

fixing typo

add UnmarshalJSON method

cannot recursively unmarshal something

fail more gracefully

using intermediate struct for conversion

adding unit tests
@jackspirou
Copy link
Contributor Author

K, made changes and added tests.

@jackspirou
Copy link
Contributor Author

@bradrydzewski does this look good now with the suggested changes?

bradrydzewski added a commit that referenced this pull request Feb 24, 2016
Add intermediate netrc struct and custom JSON marshaling methods to Netrc
@bradrydzewski bradrydzewski merged commit 51f07d3 into drone:master Feb 24, 2016
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.

3 participants