Xuanwo merged PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apac
wgtmac commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2580095022
I think this PR is ready to merge. @Fokko @Xuanwo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to
wgtmac commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2566233077
> pre-commit only runs clang-format, not clang-tidy - I think it'd still be
useful even without the PR comment?
That makes sense! Let me enable it for now. We can improve or discard
lidavidm commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2566143404
I think it's a security issue: the `write` permission doesn't discriminate
between who opened the PR or where you can write to, so someone could open a
malicious PR to write to the repos
wgtmac commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2566113136
Thanks @lidavidm for providing the detail! After reading relevant
documentation, I agree that it is really tricky to workaround the token
permission. Without comments on PR, the cpp-linter
lidavidm commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2566091765
Note that pull_request_target (1) only runs from the base branch, not the PR
and (2) doesn't check out the PR HEAD (it gets the base branch) so it's not
suitable for this by default. (Yo
lidavidm commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2566084541
It seems since the PR comes from a fork, Github will never let you get
anything but read access.
https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-
lidavidm commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2566084128
The token still says only read permissions
https://github.com/apache/iceberg-cpp/actions/runs/12554533849/job/35003344629?pr=20#step:1:17
--
This is an automated message from the Apac
wgtmac commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2566081248
> It appears either the token doesn't have comment permissions or the token
isn't being passed into the action
I have added permission for `pull-requests: write` but still hit the
f
lidavidm commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2566068656
It appears either the token doesn't have comment permissions or the token
isn't being passed into the action
--
This is an automated message from the Apache Git Service.
To respond to
wgtmac commented on code in PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#discussion_r1899875630
##
api/iceberg/table.h:
##
@@ -20,15 +20,24 @@
#pragma once
#include
-#include
+
Review Comment:
I have inserted a build step before the cpp-linter to create t
raulcd commented on code in PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#discussion_r1899439993
##
api/iceberg/table.h:
##
@@ -20,15 +20,24 @@
#pragma once
#include
-#include
+
Review Comment:
I've been able to reproduce the memory error locally:
```
raulcd commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2565246479
> cpp-linter/cpp-linter-action@v2.13.4 is not allowed to be used in
apache/iceberg-cpp.
Thanks for trying!
--
This is an automated message from the Apache Git Service.
To resp
wgtmac commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2565241547
https://github.com/apache/iceberg-cpp/actions/runs/12544655200
cpp-linter/cpp-linter-action@v2.13.4 is not allowed to be used in
apache/iceberg-cpp.
--
This is an automated messag
wgtmac commented on code in PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#discussion_r1899408870
##
.github/workflows/cpp-linter.yml:
##
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See th
raulcd commented on code in PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#discussion_r1899401009
##
.github/workflows/cpp-linter.yml:
##
@@ -0,0 +1,51 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See th
wgtmac commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2564639921
> Hmm, in that screenshot, 'memory not found' seems like a spurious error?
Will this be noisy?
I have also noticed that. I need to learn more about its configuration and
improve it
lidavidm commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2564236592
Hmm, in that screenshot, 'memory not found' seems like a spurious error?
Will this be noisy?
--
This is an automated message from the Apache Git Service.
To respond to the message, ple
wgtmac commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2563372344
Oops, my bad. I just copied pre-commit.yml and edited the wrong file. Now it
is fixed :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please lo
zhjwpku commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2563351921
> Is this something worth adding? @Fokko @raulcd @zhjwpku
+1
Is there something wrong with github? I see you are putting cpp-linter in
pre-commit.yml while pre-commit in cpp-
wgtmac commented on PR #20:
URL: https://github.com/apache/iceberg-cpp/pull/20#issuecomment-2563327729
Is this something worth adding? @Fokko @raulcd @zhjwpku
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL ab
21 matches
Mail list logo
