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

YAML parser implementations #74

Merged
merged 14 commits into from
May 12, 2021
Merged

YAML parser implementations #74

merged 14 commits into from
May 12, 2021

Conversation

ntrp
Copy link
Contributor

@ntrp ntrp commented Feb 21, 2020

Initial port, I just copied the old code and fixed compilation issues, will refine it since the base API changed since we implemented it.

My Scala knowledge sucks so if you have any suggestions they are welcome since currently I have the feeling I am using the Scala APIs totally wrong.

The test is failing (and it's poorly written), will refactor and fix in the next days.

@ntrp ntrp requested a review from simplesteph February 21, 2020 19:46
project/metals.sbt Outdated Show resolved Hide resolved
Copy link
Contributor

@simplesteph simplesteph left a comment

Choose a reason for hiding this comment

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

Thanks for this first draft, some early comments

I would need some more documentation on how the format works, as well as edits to CHANGELOG.md and README.md (in the bottom).

build.sbt Outdated Show resolved Hide resolved
project/metals.sbt Outdated Show resolved Hide resolved
@ntrp
Copy link
Contributor Author

ntrp commented May 2, 2021

So I managed to find some time and armed with some Haskell concepts I aquired in the meantime I am now finally integrating the changes..

I have to fix all the tests because I needed to refactor how the correct parser is selected. There are still some open points like:

  • is this implementation satisfiying (I know is a little Java biased)
  • Should we add some additional flexibility like forcing the use of only one source?
  • Do you have any implementation preference (not being a Scala developer I write Java biased code)

@ntrp ntrp requested a review from simplesteph May 2, 2021 12:00
@ntrp
Copy link
Contributor Author

ntrp commented May 2, 2021

I will edit the README, CHANGELOG, and others when the PR is ready for review

@ntrp
Copy link
Contributor Author

ntrp commented May 4, 2021

@simplesteph whenever you have a moment, have a look on it and give me some feedback so I can complete the PR and finally merge

@simplesteph
Copy link
Contributor

Looks great! Can you merge the changes from master please?

@ntrp
Copy link
Contributor Author

ntrp commented May 10, 2021

Ok I have rebased against master and now I am fixing the tests

@ntrp ntrp marked this pull request as ready for review May 10, 2021 20:54
Copy link
Contributor

@mbaechler mbaechler left a comment

Choose a reason for hiding this comment

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

Hi,
Your contribution looks nice.
I have some minor comments if you don't mind looking at them.

README.md Outdated Show resolved Hide resolved
ksmAcls,
ParsingExceptions
ParsingExceptions,
ksmAcls
}
import kafka.security.auth.{Acl, Resource}

object SourceAclResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

In Scala, we usually build an ADT (you can find a lot of documentation on the web) to express different status.
Here, I would write:

sealed trait SourceAclResult
case class Success(result) extends SourceAclResult
case class Failure(errors: List[ParserException]) extends SourceAclResult

you can keep the monoid, of course, but it will be much easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used this pattern to make it possible to easily use combineAll to merge the list of ACLs, can that be done with that approach?

If yes could you suggest how?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe something like that would work:

implicit val monoid: Monoid[SourceAclResult] = new Monoid[SourceAclResult] {
    override def empty: Success = Success.empty
    override def combine(
        x: SourceAclResult,
        y: SourceAclResult
    ): SourceAclResult = (x, y) match {
      case (Success(_), Failure(errors)) =>
        Failure(errors)
      case (Failure(errors), Success(_)) =>
        Failure(errors)
      case (Failure(errors1), Failure(errors2)) =>
        Failure(errors1 ++ errors2)
      case (Success(result1), Success(result2)) =>
        Success(result1 ++ result2)
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried various options with ADTs but that approach was making the whole thing more complicated in some cases because I don't want to pattern match always. For example in tests I would had to use asInstanceOf I guess. I also tried a leaner approach by defining SourceAclResult as a Either[ParsingExceptions, KsmAcls] but that broke my monoid extension and I did not know how to fix it (error where not combined anymore).

In the end I simply refactored the monoid implementation to be a little more readable and I think it looks pleasant now.

ntrp and others added 3 commits May 11, 2021 14:55
@ntrp ntrp requested a review from trobert May 11, 2021 13:16
@ntrp ntrp requested a review from mbaechler May 11, 2021 13:16
@simplesteph simplesteph merged commit 11f2001 into conduktor:master May 12, 2021
@simplesteph
Copy link
Contributor

Thanks a lot for this PR !

@ntrp
Copy link
Contributor Author

ntrp commented May 12, 2021

Next on the list I will try to merge back directory support as soon as I have some time

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