Skip to content

Comments

AVRO-3411: Support VSCode devcontainer#1557

Merged
iemejia merged 4 commits intoapache:masterfrom
zcsizmadia:avro-3411-support-devcontainer
Feb 25, 2022
Merged

AVRO-3411: Support VSCode devcontainer#1557
iemejia merged 4 commits intoapache:masterfrom
zcsizmadia:avro-3411-support-devcontainer

Conversation

@zcsizmadia
Copy link
Contributor

Make sure you have checked all steps below.

Jira

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@iemejia iemejia self-requested a review February 24, 2022 13:49
Copy link
Member

@iemejia iemejia 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 bringing this one in @zcsizmadia !
I let some minor comments.

@@ -0,0 +1,62 @@
# Set default behavior to automatically normalize line endings.
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this? Specially all the 'auto', isn't auto the default?
Should we cover the formats we don't care about e.g. doc/pdf/rtf?
I don't remember where we are towards line endings. @RyanSkraba maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

There was a discussion on this about AVRO-3150 -- some tests were failing due to an overzealous checkstyle check IIRC.

Is there something that fails if this .gitattributes isn't present? I'm not familiar with all the attributes, I admit! What does diff and astextplain do?

I have no objection to enforcing that script files are CRLF/LF no matter where they're checked out!

Copy link
Contributor

@RyanSkraba RyanSkraba left a comment

Choose a reason for hiding this comment

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

I would love to take a closer look at this -- to be honest, I've always kind of disliked the uberjar, but this is a pretty neat feature!

@zcsizmadia
Copy link
Contributor Author

I heavily use devcontainer for other multi language/multi platform projects. AFAIK VSCode is the most used editor for such projects.

I like the fact that e.g. when I developed the extra codecs for C# , I could just generate the interop data for all the other languages, so my C# tests were running againsta ll the compression codecs generated by the other languages.
Just simply ran ./build.sh interop...generate, without knowing anything about Ruby, PHP, or Java.

@zcsizmadia
Copy link
Contributor Author

It is possible to have devcontainer config for each languages seperately, however since the dockerfile has all the tools and all the project depend on each other via the interop tests, I thought having full container setup for the whole project (based on the already existing dockerfile) makes more sense

Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM

I validated it in a local container on mac and it works.

I had some issues with the C# project import but we can fix those afterwards, merging this is great for the benefit of everyone who can now contribute in an easy reproducile way. Excellent work @zcsizmadia !

@iemejia iemejia merged commit 72e1135 into apache:master Feb 25, 2022
RyanSkraba pushed a commit that referenced this pull request Feb 25, 2022
* Add devcontainer file

* Add info to BUILD.md

* Fix whitespaces

* Add only scripts to gitattributes

Co-authored-by: Zoltan Csizmadia <CsizmadiaZ@valassis.com>
@zcsizmadia zcsizmadia deleted the avro-3411-support-devcontainer branch February 25, 2022 12:58
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