[Platforms] Community Outreach role

Proposal
Create a community outreach role within the Platforms WG. This proposal comes from what @abangser described as a possible role in yesterday's meeting, but I thought that to achieve that one, there'd be a need for a bigger coordination.
Motivation
There are three main reasons why I think this is much needed:

The whitepaper popularity has shown how much the community is expecting vendor-neutral literature on the subject of Platform engineering. Right now, it is not the easiest to figure out what the WG is working on or what it has to say, as the website is not as known as it could be.
The WG wants to talk to end-users regularly. In order to make sure we can have a constant flow of people sharing their stories, people must know about the WG existence and that sharing their stories is a possibility.
The WG will also want to reach out to the community for initiatives like the survey, which requires a more structured communications

What would this role do?

Structure a communication strategy and channels for outreach (LinkedIn? Newsletter?).
Give visibility to the initiatives the WG is working on
Regularly publish content to update audiences on what the WG is working on and share outcomes
Actively engage with the community

Who could do this role?
Naturally, I volunteer to take care of these issues. But of course there may be somebody who's up for the task too. I have a limited experience doing this as part of a CNCF WG, have only done it for companies.

I love the idea of this type of role. It would probably be best to start off by defining this as a team, even if that team only has one member at the start, just to make sure that this is a role that can be easily passed on if needed.
It might even be beneficial to talk about having such a team on a TAG level, so one could share knowledge across the TAG. Then someone from the OpenGitOps project could join the team as the designated community outreach person for the project, and then you (@jorgelainfiesta) could be the dedicated person for the platforms working group.
I'll try to remember to mention it in both the TAG and OpenGitOps meeting.

This is so amazing @jorgelainfiesta, thanks for turning a half baked suggestion into an actionable role!
I think @roberthstrand has added some key points and his suggestions make the idea work well within the CNCF structures which will hopefully enable a broader and longer lasting impact. But I also don't think it has to be limiting if we don't immediately have an interested party from each WG. IMO setting the structure up at the TAG level makes sense, but I don't think you should be limited on getting started withe the WG Platform until that is matured.
I wonder if next steps are:

Raise at next TAG meeting (May 3)
Open a PR to this repo with a new directory to manage associated documents

I don't think these are necessarily dependent on each other, but raising at the meeting may make the PR easier to understand / review for the team members.

Thank you for starting this @jorgelainfiesta! To date outreach activity for the TAG and its WGs has been informal and we haven't made much explicit effort to reach our audience, that would be welcome! @lianmakesthings has also stepped up to help in this area and she may be able to support this too.
Agree with @roberthstrand and @abangser's sentiment that the initiative applies to the TAG generally, but it's reasonable IMHO to start by focusing on WG Platforms and go from there. @roberthstrand it would be great to bring this up in GitOps too. I'll label this issue to be discussed in the next WG Platforms and TAG meetings.
FYI @jorgelainfiesta, here are some existing presences we have:

TAG website - https://github.com/cncf/tag-app-delivery/blob/main/website/README.md
TAG Twitter account - @CNCFTAGApp
TAG community page: https://community.cncf.io/tag-app-delivery/

We also can request content be published on the CNCF blog pretty easily.
Thanks again!

Thank you @jorgelainfiesta for suggesting this, we're trying to integrate community outreach into our work and appreciate the support. I'll close this issue as complete as we're pursuing the items like Web pages and newsletters now and have added some leads to the TAG to help.


Refreshing the application deployments with a refetchable GraphQL fragment doesn't work

In the Deployed Applications tab of the Device page, when the user clicks on the Deploy/Start/Stop buttons, a corresponding GraphQL mutation is run to deploy/start/stop the deployment.
After the mutation succeeds, the frontend tries to re-fetch the GraphQL fragment about deployedApplications.
Using a Relay refetchable fragment for the deployedApplications is a good approach when working with Relay..
However Relay uses the node query instead of the initial device query when refetching the fragment, and the node query doesn't currently support fetching relationships on the node resource (applicationDeployments of the device): see also https://github.com/ash-project/ash_graphql/issues/99
When the node query returns without deployments, all deployments previously visible disappear from the UI.
The user has to refresh the page (which uses the device GraphQL query) to correctly fetch the existing deployments and see them again.

fixed in #736


legacy flag does not work from version v0.0.30 when the login method is spn

How to reproduce it
kubelogin get-token --tenant-id <> --server-id <> --client-id <> --client-secret <> --login spn --legacy
Expected outcome:
token with audience field has "spn:" prefix
It works fine  on version v0.0.29 and lower

hmm. i think that is not supported in the new sdk (azidentity) that we migrated in v0.0.30. Are you managing your own k8s AAD configuration? would it be possible to migrate the server side?

Changing the server side will require api restart and we are seeing if we can avoid that
This is failing only for serviceprincipal flow, the deviceToken flow is still working as expected

are you okay to stay on v0.0.29 and work on your backend migration? I want to avoid reverting that change for an outdated sdk

Any reason why is the device code flow not switched to azidentity, it is still using adal library - https://github.com/Azure/kubelogin/blob/master/pkg/token/devicecode.go
I am not asking you to revert it, but can we use legacy flag to decide to use azidentity or adal for getting the token ?

i might migrate devicelogin to azidentity when i have free resources :)
your proposal is fine, but it still requires us to have outdated adal sdk.

Changing the server side will require api restart and we are seeing if we can avoid that

It's worth than that actually, it requires control plane rebuild and takes 30 minutes, so it's not an option

i will see if I can bring back the old code when --legacy is enabled next week


[HOLD] [$250] Migrate TimezoneSelectPage.js to function component

Class Component Migration
Filenames

[ ] src/pages/settings/Profile/TimezoneSelectPage.js

type: React.Component
has state values: true
has refs: false
has context: false
lifecycle methods: componentDidUpdate

Task

We currently have some class components in our codebase that we would like to refactor to a function component.
Here's a link with some general advice on how to refactor a class component to a function component: https://react.dev/reference/react/Component#alternatives
If you need additional guidance, please ask in #expensify-open-source
Test for any regressions and verify that there are no breaking changes

⚠ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

Heads up! The pricing for this issue has been adjusted based on the scope of the work and the fact that no complex bug investigations or proposal are required.

I'd like to work on this issue when it goes to external

I'd love to work on this.

I would like to work on this.

I am able to work on this task.

I like to work on this task.

i will work on this

Hey! I’m Michał from Software Mansion, an expert agency, and I’d like to work on this issue

I would to work on this

Dibs

Contributor details
Expensify account email<EMAIL_ADDRESS>Upwork profile link: https://www.upwork.com/freelancers/~0181b4de63bd872ee3

Contributor details
Your Expensify account email<EMAIL_ADDRESS>Upwork Profile Link: https://www.upwork.com/freelancers/~0181b4de63bd872ee3

Hey!
I would like to work on this
How to submit the proposal to work?

Contributor details
Your Expensify account email<EMAIL_ADDRESS>Upwork Profile Link: https://www.upwork.com/freelancers/~0148a166237a9d872b

Hello Expensify Team, I would love to work on this issue
here are my contributor details
Expensify Email<EMAIL_ADDRESS>Upwork URL: www.upwork.com/freelancers/saqixpro

Hello Expensify Team, I would love to work on this issue
here are my contributor details
Expensify Email<EMAIL_ADDRESS>Upwork URL: www.upwork.com/freelancers/saqixpro

This issue has been assigned. Proposals can be made to issues with the Help Wanted label (please always look before proposing). Read the contributing guidelines and check this list.

@marcaaron is BZ needed here for anything? I'm thinking no, since this isn't a bug, and C and C+ are either with Software Mansion or Callstack (so no BZ payment is needed on this issue). But want to make sure you agree before I unassign?

Contributor details
Your Expensify account email<EMAIL_ADDRESS>Upwork Profile Link: https://www.upwork.com/freelancers/~01d3b46af0e62d447c

@joekaufmanexpensify oh man - I actually don't know anything about how payments work at all 😬 😅

Ah, you're good! That part I am sure about. They will both be paid outside of Upwork/NewDot (per this SO).

I just meant whether there's anything else that would be needed here from a BZ perspective. As since this isn't a bug, there will be no BZ checklist to process here afterwards. So I don't think there is anything for BZ to do here. So going to unassign for now!
If that's not correct (or if anything BZ related is required at any point), feel free to add me back, and happy to help!

I don't think there is anything for BZ to do here

Feel free to close the issue if you have confirmed there is no further action.


Release model on Hugging Face

Hello @haodong2000 and team 🤗
I'm Niels and work as part of the open-source team at Hugging Face. I discovered through AK's daily papers: https://huggingface.co/papers/2409.18124. The paper page lets people discuss about your paper and lets them find artifacts about it (your model for instance) you can also claim the paper as yours which will show up on your public profile at HF.
Would you like to host the model you've pre-trained on https://huggingface.co/models? Hosting on Hugging Face will give you more visibility. We can add tags in the model cards so that people find the models easier, link it to the paper page, etc.
If you're down, leaving a guide here. If it's a PyTorch model, you can use PyTorchModelHubMixin class which adds from_pretrained and push_to_hub to the model which lets you to upload the model and people to download and use models right away.
If you do not want this and directly want to upload model through UI or however you want, people can also use hf_hub_download.
After uploaded, we can also link the models to the paper page (read here) so people can discover your model.
You can also build a demo to your model on Spaces - I see you already have a draft at - https://huggingface.co/spaces/haodongli/Lotus- we can provide you an A100 grant.
What do you think?
Kind regards,
Niels

I will do it soon, probably tomorrow, today is a little tired, just after a
8hours flight.
Thanks for your advice!
On Fri, 4 Oct 2024 at 15:51, NielsRogge @.***> wrote:

Hello @haodong2000 https://github.com/haodong2000 and team 🤗
I'm Niels and work as part of the open-source team at Hugging Face. I
discovered through AK's daily papers:
https://huggingface.co/papers/2409.18124. The paper page lets people
discuss about your paper and lets them find artifacts about it (your model
for instance) you can also claim the paper as yours which will show up on
your public profile at HF.
Would you like to host the model you've pre-trained on
https://huggingface.co/models? Hosting on Hugging Face will give you more
visibility. We can add tags in the model cards so that people find the
models easier, link it to the paper page, etc.
If you're down, leaving a guide here
https://huggingface.co/docs/hub/models-uploading. If it's a PyTorch
model, you can use PyTorchModelHubMixin
https://huggingface.co/docs/huggingface_hub/package_reference/mixins#huggingface_hub.PyTorchModelHubMixin
class which adds from_pretrained and push_to_hub to the model which lets
you to upload the model and people to download and use models right away.
If you do not want this and directly want to upload model through UI or
however you want, people can also use hf_hub_download
https://huggingface.co/docs/huggingface_hub/en/guides/download#download-a-single-file
.
After uploaded, we can also link the models to the paper page (read here
https://huggingface.co/docs/hub/en/model-cards#linking-a-paper) so
people can discover your model.
You can also build a demo to your model on Spaces
https://huggingface.co/spaces - I see you already have a draft at -
https://huggingface.co/spaces/haodongli/Lotus- we can provide you an A100
grant.
What do you think?
Kind regards,
Niels
—
Reply to this email directly, view it on GitHub
https://github.com/EnVision-Research/Lotus/issues/4, or unsubscribe
https://github.com/notifications/unsubscribe-auth/AQFCU4T2A73WSXACY2PV2O3ZZ3WTBAVCNFSM6AAAAABPMQCV5WVHI2DSMVQWIX3LMV43ASLTON2WKOZSGU3DOMRSGE3DKOI
.
You are receiving this because you were mentioned.Message ID:
@.***>

Dear Niels,
So excited to hear your A100 grant! Ours demos are now available, and we are using L40 GPU. Here are the links:

https://huggingface.co/spaces/haodongli/Lotus_Depth
https://huggingface.co/spaces/haodongli/Lotus_Normal

Also, I have applied for the A100 access by sending emails to<EMAIL_ADDRESS>and applied for the GPU Grant by opening discussions on each demo:

https://huggingface.co/spaces/haodongli/Lotus_Depth/discussions/4
https://huggingface.co/spaces/haodongli/Lotus_Normal/discussions/1

Thanks again for your kind notice and the wonderful GPU Grant! I deeply appreciated it.
Best,

Thank you for creating these hf spaces! they are currently in an error state. Lotus Normal has a runtime error with bitsandbytes not supporting GPU and Lotus Depth is 404 (probably not publically viewable)

Thanks for notice, I've fixed them. Both demos should be working now.
Sorry for my carelessness yesterday, and thanks again for your kind GPU Grant!
Best,

Hi @haodong2000 awesome work, thanks for sharing everything on the hub, linking them to the paper! https://huggingface.co/papers/2409.18124 looks awesome now👌
We could give you a grant to support your research, so that you can leverage ZeroGPU. ZeroGPU is a new solution where various demos share the same underlying hardware, and GPUs get dynamically assigned based on demand.
Will ping @merveenoyan regarding this

Thanks so much for linking to the paper page and the GPU Grant!
Yes, I have already posted the huggingface spaces on the media! Hopefully AK will repost or quote my tweet!
Also please consider group the checkpoints in a collection @BlingHe , if convenient.
Thanks,

Thank you for hosting apps on Huggingface Spaces @haodong2000, great work!!
It would be great to also have these Gradio apps available locally in your repository. For example, the DepthCrafter repository includes reproducible code for the Spaces Gradio app and they have linked it in the README along with instructions on how to run it. This improves the project's accessibility and discoverability among the community.
Gradio demo on Tencent's DepthCrafter repo: https://github.com/Tencent/DepthCrafter?tab=readme-ov-file#-gradio-demo

Hi Niels,
I have just added the local gradio demo into this repo.
Thanks for your kind reminder and the support from Huggingface community!
Best,

Awesome, thank you so much for moving quickly on this! 🤗

Thanks so much for collaborating, I'm going to close this issue, let us know if you have any future releases which we can help with. 🤗


doc(readme): remove the table of content

The table of content is useless in case of a small README. That's our case!

Coverage remained the same at 98.183% when pulling 40e3c44c2893451ef270135c22d777185d8d1087 on RomainLanz:patch-1 into c0fc70738dcae894819cfa996c98001cc72d8e93 on adonisjs:develop.


Remove leading underscores from C include guards

Hey,
I'm trying to reduce the scope of undefined behaviour in the generated C code (hopefully leading to more portable code), and thought I'd start off with this quick change!
According to the C specification (7.1.3 Reserved Identifiers), include guards should not start with leading underscores:

Each header declares or defines all identifiers listed in its associated subclause, and optionally declares or defines identifiers listed in its associated future library directions subclause and identifiers which are always reserved either for any use or for use as file scope identifiers.

All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.
All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces.

According to this, identifiers beginning with underscores are technically reserved for implementation. See this StackOverflow post or Wikipedia article for more info.
This changes the include guard format from __FILE_H__ to FILE_H, thereby reducing the scope of undefined behaviour.
Edit: Seems like this is one of the linter errors raised in the MISRA bug, so I guess this also fixes the MISRA 2012 Rule 21.1 violations in the header files.

Coverage remained the same at 93.317% when pulling 43d836d2730f33d5c8f4969aee3c0391f45f6158 on karlding:undefined_behaviour/include_guards into 6ea60295c2b18428f1042db35182fd2c74196ad1 on eerimoq:master.

Coverage remained the same at 93.317% when pulling 43d836d2730f33d5c8f4969aee3c0391f45f6158 on karlding:undefined_behaviour/include_guards into 6ea60295c2b18428f1042db35182fd2c74196ad1 on eerimoq:master.

Hello,
thanks, looks good!
I guess the goal of the generated C source is that it is correct, easy to read, reasonably efficient in runtime, and reasonably portable. All of this to make people want to use it in their projects.


fix: ResetDB script crash when running on clean database

Description/Purpose
A newly created database would not have any colums by default. The script tries to access a column before it was created, this condition results in script getting crashed. Anyways, the variable which was defined was unsed, so simply removing it fixes the issue
Stack-trace of the issue
➜  backend git:(v4-dev) ✗ node dist/scripts/ResetDB.js
2022-10-30 21:33:26 +05:30 [info]: V1 access code generator is unavailable
2022-10-30 21:33:26 +05:30 [info]: Starting reset script DB
/Users/divyansh/Documents/GitHub/rural-crowdsourcing-toolkit/server/common/node_modules/pg-protocol/dist/parser.js:287
        const message = name === 'notice' ? new messages_1.NoticeMessage(length, messageValue) : new messages_1.DatabaseError(messageValue, length, name);
                                                                                ^

error: select * from "server_user" - relation "server_user" does not exist
    at Parser.parseErrorMessage (/Users/divyansh/Documents/GitHub/rural-crowdsourcing-toolkit/server/common/node_modules/pg-protocol/dist/parser.js:287:98)
    at Parser.handlePacket (/Users/divyansh/Documents/GitHub/rural-crowdsourcing-toolkit/server/common/node_modules/pg-protocol/dist/parser.js:126:29)
    at Parser.parse (/Users/divyansh/Documents/GitHub/rural-crowdsourcing-toolkit/server/common/node_modules/pg-protocol/dist/parser.js:39:38)
    at Socket.<anonymous> (/Users/divyansh/Documents/GitHub/rural-crowdsourcing-toolkit/server/common/node_modules/pg-protocol/dist/index.js:11:42)
    at Socket.emit (node:events:527:28)
    at addChunk (node:internal/streams/readable:315:12)
    at readableAddChunk (node:internal/streams/readable:289:9)
    at Socket.Readable.push (node:internal/streams/readable:228:10)
    at TCP.onStreamRead (node:internal/stream_base_commons:190:23) {
  length: 110,
  severity: 'ERROR',
  code: '42P01',
  detail: undefined,
  hint: undefined,
  position: '15',
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  schema: undefined,
  table: undefined,
  column: undefined,
  dataType: undefined,
  constraint: undefined,
  file: 'parse_relation.c',
  line: '1395',
  routine: 'parserOpenTable'
}

@microsoft-github-policy-service agree


Game Capped at 60 FPS

I have a question regarding this library, used with a video game from 2009 called Battlestations: Pacific. It's 32-bit, it uses DirectX 9 and Games for Windows LIVE (GFWL), on Windows 10.
We worked on a mod project for that game that involved creating a custom version of the XLiveLessNess project, with the aim of restoring multiplayer functionality after GFWL got disbanded, and some other stuff. In our case, that involved creating a custom xlive.dll file that the game loads at startup, that included all our required modifications.
We were successfully able to achieve most of our goals with that project, however we noticed an oddity;
For some reason, whenever the game loads a custom xlive.dll file, it appears to cap it's maximum framerate at 60 FPS during normal runtime... This doesn't only happen with our xlive.dll variant, but also with other variations of Xliveless that are commonly used with other games, such as GTA 4. That was extremely puzzling to us, as the xlive.dll file we created didn't make any modifications to the way the game handled graphics or drawing stuff to the screen. Without a custom xlive.dll, the game returns to running normally at well above 60 FPS.
After doing some searching online, I saw several posts about other games from the simillar time, suggesting this DXVK project that is often used with DirectX games.
I was wondering whether it would be possible to change the max framerate of a DirectX 9 game using this library, which appears to somehow be capped by the custom xlive.dll file.
I followed your tutorial on setting up the Apitrace 32-bit debug logging, and have put a link to a trace of the game down below under the system information. I simply entered & exited the game, while it was running at a capped 60 FPS. I ran this on the latest version of DXVK Apitrace.
I have also tried running the game with the latest DXVK release v2.3.1, and I noticed it produced another debug log. I have also attached that log to this post underneath the Apitrace log, in case that's helpful.
Thanks for reading my post, any guidance is appreciated. Let me know if anything else is required in order to give you a better idea of the situation.
Game information

Name: Battlestations: Pacific (2009)
DirectX: 9
Architecture: 32-bit
OS: Windows 10

System information

GPU: Nvidia GTX 1060
Driver: 546.17
DXVK version: 2.3.1

Trace/Log files
https://drive.google.com/file/d/1UnoPn4FCp25HVsgcTDnFSLeNuKfdHoC2/view?usp=sharing
battlestationspacific_d3d9.log

I'm not personally seeing an issue so far. Here i am running the game with xliveless from pcgamingwiki and vsync off.

screenshots

I'm not personally seeing an issue so far. Here i am running the game on Windows 11 with xliveless from pcgamingwiki and vsync off.
screenshots

Thank you for your reply;
I sincerely apologize for the confusion, but I found out what the issue was. XLiveLessNess limits FPS, and ours was outdated so we didn't have access to the FPS changing functions.
Cheers!


Fails to compile due to conflict with STL's is_empty

It fails to compile for me using GCC 6.3.0 on SmartOS (Solaris derivative). Renaming the is_empty() function to something else fixes the problem. The conflict seems to be with the STL's std::is_empty
mordac ~/passvet/quotient-filter>gmake
g++     test.cc qf.c   -o test
test.cc: In function ‘void qf_consistent(quotient_filter*)’:
test.cc:103:9: error: reference to ‘is_empty’ is ambiguous
     if (is_empty(elt)) {
         ^~~~~~~~
In file included from test.cc:8:0:
qf.c:127:20: note: candidates are: bool is_empty(uint64_t)
 static inline bool is_empty(uint64_t elt)
                    ^~~~~~~~
In file included from /usr/local/include/c++/6.3.0/bits/move.h:57:0,
                 from /usr/local/include/c++/6.3.0/bits/stl_pair.h:59,
                 from /usr/local/include/c++/6.3.0/bits/stl_algobase.h:64,
                 from /usr/local/include/c++/6.3.0/bits/stl_tree.h:63,
                 from /usr/local/include/c++/6.3.0/set:60,
                 from test.cc:13:
/usr/local/include/c++/6.3.0/type_traits:701:12: note:                 template<class _Tp> struct std::is_empty
     struct is_empty
            ^~~~~~~~
In file included from /usr/local/include/c++/6.3.0/cassert:44:0,
                 from test.cc:15:
test.cc:113:15: error: reference to ‘is_empty’ is ambiguous
       assert(!is_empty(prev));
               ^
In file included from test.cc:8:0:
qf.c:127:20: note: candidates are: bool is_empty(uint64_t)
 static inline bool is_empty(uint64_t elt)
                    ^~~~~~~~
In file included from /usr/local/include/c++/6.3.0/bits/move.h:57:0,
                 from /usr/local/include/c++/6.3.0/bits/stl_pair.h:59,
                 from /usr/local/include/c++/6.3.0/bits/stl_algobase.h:64,
                 from /usr/local/include/c++/6.3.0/bits/stl_tree.h:63,
                 from /usr/local/include/c++/6.3.0/set:60,
                 from test.cc:13:
/usr/local/include/c++/6.3.0/type_traits:701:12: note:                 template<class _Tp> struct std::is_empty
     struct is_empty
            ^~~~~~~~
test.cc:117:10: error: reference to ‘is_empty’ is ambiguous
     if (!is_empty(elt)) {
          ^~~~~~~~
In file included from test.cc:8:0:
qf.c:127:20: note: candidates are: bool is_empty(uint64_t)
 static inline bool is_empty(uint64_t elt)
                    ^~~~~~~~
In file included from /usr/local/include/c++/6.3.0/bits/move.h:57:0,
                 from /usr/local/include/c++/6.3.0/bits/stl_pair.h:59,
                 from /usr/local/include/c++/6.3.0/bits/stl_algobase.h:64,
                 from /usr/local/include/c++/6.3.0/bits/stl_tree.h:63,
                 from /usr/local/include/c++/6.3.0/set:60,
                 from test.cc:13:
/usr/local/include/c++/6.3.0/type_traits:701:12: note:                 template<class _Tp> struct std::is_empty
     struct is_empty
            ^~~~~~~~
test.cc: In function ‘uint64_t genhash(quotient_filter*, bool, std::set<long unsigned int>&)’:
test.cc:145:11: error: reference to ‘is_empty’ is ambiguous
       if (is_empty(get_elem(qf, probe))) {
           ^~~~~~~~
In file included from test.cc:8:0:
qf.c:127:20: note: candidates are: bool is_empty(uint64_t)
 static inline bool is_empty(uint64_t elt)
                    ^~~~~~~~
In file included from /usr/local/include/c++/6.3.0/bits/move.h:57:0,
                 from /usr/local/include/c++/6.3.0/bits/stl_pair.h:59,
                 from /usr/local/include/c++/6.3.0/bits/stl_algobase.h:64,
                 from /usr/local/include/c++/6.3.0/bits/stl_tree.h:63,
                 from /usr/local/include/c++/6.3.0/set:60,
                 from test.cc:13:
/usr/local/include/c++/6.3.0/type_traits:701:12: note:                 template<class _Tp> struct std::is_empty
     struct is_empty
            ^~~~~~~~
gmake: *** [<builtin>: test] Error 1

Thanks for the report! Sorry I didn't see it earlier. I've fixed the build:
To github.com:vedantk/quotient-filter.git
54539e6..a65a571  master -> master


Add --ignore CLI option to ignore certain symbol patterns

Hello!
Since a recent upgrade of abi3audit, my psutil CI builds started failing because long time ago I defined some custom Py* functions which I use pretty much all over the C extension code (>200 occurrences). My take from this is that abi3audit does not recognize them as known cPython API symbols due to the fact that I used a Py* prefix. Would it be possible to add an --ignore CLI parameter in order to ignore such occurrences?
E.g. in my case python3 -m abi3audit --ignore=PyInit__psutil_posix --ignore=PyErr_SetFromOSErrnoWithSyscall ... would serve this purpose.
Thanks in advance
$ python3 -m abi3audit --verbose  dist/*-abi3-*.whl
[10:26:14] 👎 psutil-6.0.1-cp310-abi3-linux_x86_64.whl: _psutil_linux.abi3.so has non-ABI3 symbols
           ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
           ┃ Symbol                          ┃
           ┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩
           │ PyInit__psutil_posix            │
           │ PyErr_SetFromOSErrnoWithSyscall │
           └─────────────────────────────────┘
           👎 psutil-6.0.1-cp310-abi3-linux_x86_64.whl: _psutil_posix.abi3.so has non-ABI3 symbols
           ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
           ┃ Symbol                          ┃
           ┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩
           │ PyErr_SetFromOSErrnoWithSyscall │
           └─────────────────────────────────┘
           💁 psutil-6.0.1-cp310-abi3-linux_x86_64.whl: 2 extensions scanned; 0 ABI version mismatches and 3 ABI violations found

Hey @giampaolo, thanks for the feature request!
I've got mixed feelings on this: on one hand these are false positives from a stable ABI perspective, but on the other they're strongly discouraged by CPython's API:

Note: User code should never define names that begin with Py or _Py. This confuses the reader, and jeopardizes the portability of the user code to future Python versions, which may define additional names beginning with one of these prefixes.

(link: https://docs.python.org/3/c-api/intro.html#include-files)
TL;DR: I think I could be convinced here, but only if it'd be a significant refactoring challenge for psutil to align with the CPython API guidelines here, since I'm worried that others will use it to suppress "works on my machine" type ABI errors 🙂

TL;DR: I think I could be convinced here, but only if it'd be a significant refactoring challenge for psutil to align with the CPython API guidelines here

Thank you, I appreciate it, but the refactoring is not hard per-se (it's just a mass rename). And I definitively wouldn't ask you to put the burden on your tool just because the refactoring on my tool is hard (it's not). =)
Adding --ignore should be justified by a realistic use case, but honestly I can't think of any other than mine. The doc extract you quoted makes sense, so I guess I will just go ahead and rename these functions, even though I'll have to think of good name, which I don't have at the moment. :)

Sounds good! I'll leave this open in case others have other use cases where an --ignore or similar flag makes sense. Thanks for your diligence here!

Sorry, I still could use your help. I renamed PyErr_SetFromOSErrnoWithSyscall which fixes 1 warning, but I still get this error re. to PyInit__psutil_posix:
           ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
           ┃ Symbol                          ┃
           ┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┩
           │ PyInit__psutil_posix            │
           └─────────────────────────────────┘

Note: psutil on Linux has 2 extension modules, _psutil_linux and _psutil_posix:

https://github.com/giampaolo/psutil/blob/176112f837081081e2b13b4fcadd460ba9becf5f/psutil/_psutil_linux.c#L66
https://github.com/giampaolo/psutil/blob/176112f837081081e2b13b4fcadd460ba9becf5f/psutil/_psutil_posix.c#L901

I'm not sure why abi3audit complains about _psutil_posix but not _psutil_linux. Any idea?

Hmm, random guess, but is PyInit__psutil_posix present in extensions outside of _psutil_posix.so?
abi3audit allows PyInit_ symbols as long as the match the extension object's name (e.g. _foo.so becomes PyInit__foo.so), but rejects any other PyInit_ symbols. So PyInit__psutil_posix would cause errors if it's present in anything other than _psutil_posix.so.

Ah yeah, based on your CI you have PyInit__psutil_posix inside of _psutil_linux.so:
           psutil-6.0.1-cp36-abi3-manylinux_2_17_aarch64.manylinux2014_aarch64.w
           hl: _psutil_linux.abi3.so has non-ABI3 symbols
           ┏━━━━━━━━━━━━━━━━━━━━━━┓
           ┃ Symbol               ┃
           ┡━━━━━━━━━━━━━━━━━━━━━━┩
           │ PyInit__psutil_posix │
           └──────────────────────┘

This is a funny edge case 🙂 -- I'll have to see what the C ABi reference says about this! It's possible abi3audit is being too conservative about how it checks PyInit, or this may be a slight violation of the extension requirements.

From https://docs.python.org/3/extending/building.html#c.PyInit_modulename:

It is possible to export multiple modules from a single shared library by defining multiple initialization functions. However, importing them requires using symbolic links or a custom importer, because by default only the function corresponding to the filename is found. See the “Multiple modules in one library” section in PEP 489 for details.

My interpretation of this is that it's OK to have multiple PyInit_ symbols in the same module, since CPython won't touch them unless a symlink exists that matches one. I'm not sure whether your extensions are symlinked are not, but either way it seems like abi3audit's current check is too conservative there. I'll have a fix ready in a moment!


Memory leak in MLX / Metal / MPS

Bug / issue description
I am currently running into an issue of MLX / PyTorch MPS taking up too much memory during text generation. Even if I run a smaller model, e.g. a quantized Mistral-7B-Instruct, but the end of text generation my M3 Max MBP with 64GB of RAM is pretty much out of memory (see memory usage in attached images. I was wondering if there is a memory leak in MLX / MPS, as I am getting the same issue when running text generation using either MLX or PyTorch with MPS backend. Or is this an issue of Python no de-allocating memory? I do not get the same issue with LMStudio or if I generate text using PyTorch with CPU back-end (painfully slow).
To Reproduce
Create a conda environment with python=3.11 and install the following packages:
   conda install pytorch::pytorch torchvision torchaudio -c pytorch
   conda install -c conda-forge mlx mlx-lm

Include code snippet
python -m mlx_lm.generate \
    --model /path/to/checkpoint/mlx-community/Mistral-7B-Instruct-v0.2-8-bit-mlx  \
    --prompt "Write a short essay about the first generation of Pokemon games (Pokemon Red and Blue in EU /US) and their impacts on the world." \
    --max-tokens 1024

This is not a leak, it's a combination of the MLX memory cache and the fact that we don't preallocate all used memory which is causing this.
We have a working issue #742 to address memory management in MLX with precisely the goal to fix this.
In the meantime you could see reduced memory use by disabling the cache (at the cost of some generation speed):
E.g. put mx.disable_cache() in the code before you start generating text.

I'm closing this as a dup of #742. We'll use that issue to track memory use issues.


Sync with upstream

Description

Sync our ign-rendering6 branch (including the lidar development) with modifications from upstream.
I ran these:
git checkout ign-rendering6
git remote add upstream https://github.com/gazebosim/gz-rendering.git
git fetch upstream
git merge upstream/ign-rendering6

Then resolved the conflicts and tried to manually build the library as below, solving any problem along the way.
# In the main directory:
docker build -t deepx-gz-rendering .
docker run -dt --name test_gz_rendering deepx-gz-rendering:latest bash
docker exec -it test_gz_rendering bash

# In the container, mimic the commands in CI.
cd /root/gazebo/gz-rendering/build
cmake ..
cmake --build . --target install
ctest

For the details of the conflict and build problem resolutions, please see commits from me.
Motivation and Context

In general, to prepare to upgrade to Gazebo Garden. But this PR is more for the time until the upgrade is finished.
For the time after that, see #12 .
How Has This Been Tested?

Through manual build as written earlier, and CI.
Checklist

[x] Have you filled the sections above?
[x] Have you checked to ensure there aren't other open Pull Requests for the same GitHub Issues/ClickUp Task(s)?
[x] Have you documented the changes in GitHub Issues and/or ClickUp Task(s) related to this Pull Request?
[x] Have you added labels, where appropriate, to this Pull Request?

I'm getting an error while cmake --build . in gz-rendering/build
[ 11%] Building CXX object src/CMakeFiles/ignition-rendering6.dir/RenderEnginePlugin.cc.o
/root/gz-rendering/src/RenderEngineManager.cc:24:10: fatal error: gz/plugin/Loader.hh: No such file or directory
 #include <gz/plugin/Loader.hh>
          ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [src/CMakeFiles/ignition-rendering6.dir/build.make:232: src/CMakeFiles/ignition-rendering6.dir/RenderEngineManager.cc.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/Makefile2:2511: src/CMakeFiles/ignition-rendering6.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 12%] Linking CXX static library ../../../../lib/libterra.a
[ 12%] Built target terra
make: *** [Makefile:163: all] Error 2
root@9ea976c5d8b9:/root/gz-rendering/build#

It's just looking for https://github.com/gazebosim/gz-plugin basically

Speaking of which, which container are you in?

Speaking of which, which container are you in?

Oh, I'm building gz-rendering in Talos container

Oh there she is https://github.com/DeepX-inc/gz-rendering/blob/8573f9f654585de89347cd037d20ec6e946d6ac8/Dockerfile#LL54C13-L54C13

I have removed the offending library. Also wanted to try building in Talos container myself to detect other errors, but may I know how to do it? Do you just clone the branch in Talos container then build it?

I have removed the offending library. Also wanted to try building in Talos container myself to detect other errors, but may I know how to do it? Do you just clone the branch in Talos container then build it?

I think we need that library

I'm getting an error while cmake --build . in gz-rendering/build
[ 11%] Building CXX object src/CMakeFiles/ignition-rendering6.dir/RenderEnginePlugin.cc.o
/root/gz-rendering/src/RenderEngineManager.cc:24:10: fatal error: gz/plugin/Loader.hh: No such file or directory
 #include <gz/plugin/Loader.hh>
          ^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [src/CMakeFiles/ignition-rendering6.dir/build.make:232: src/CMakeFiles/ignition-rendering6.dir/RenderEngineManager.cc.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/Makefile2:2511: src/CMakeFiles/ignition-rendering6.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 12%] Linking CXX static library ../../../../lib/libterra.a
[ 12%] Built target terra
make: *** [Makefile:163: all] Error 2
root@9ea976c5d8b9:/root/gz-rendering/build#

I think it's because I am not using the deepx-gz-rendering image.

Ah so this error is caused by the namespace change
root@9ea976c5d8b9:/root/gazebo# ls /usr/include/ignition/plugin1/ignition/plugin/
EnablePluginFromThis.hh  Factory.hh  Loader.hh  PluginPtr.hh  RegisterMore.hh       SpecializedPluginPtr.hh  config.hh  loader
Export.hh                Info.hh     Plugin.hh  Register.hh   SpecializedPlugin.hh  WeakPluginPtr.hh         detail     utility.hh

The header is under "ignition/plugin/Loader.hh"

Ah it's because I'm on 1.3.0
root@9ea976c5d8b9:/root/gazebo# dpkg -l | grep ignition-plugin
ii  libignition-plugin:amd64                           1.3.0-1~focal                         amd64        Gazebo Plugin Library - Shared library
ii  libignition-plugin-dev:amd64                       1.3.0-1~focal                         amd64        Gazebo Plugin Library - Development files
root@9ea976c5d8b9:/root/gazebo# apt install libignition-plugin-dev
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following additional packages will be installed:
  libignition-plugin
The following packages will be upgraded:
  libignition-plugin libignition-plugin-dev
2 upgraded, 0 newly installed, 0 to remove and 563 not upgraded.
Need to get 199 kB of archives.
After this operation, 406 kB of additional disk space will be used.
Do you want to continue? [Y/n] y
Get:1 http://packages.osrfoundation.org/gazebo/ubuntu-stable focal/main amd64 libignition-plugin-dev amd64 1.4.0-1~focal [157 kB]
Get:2 http://packages.osrfoundation.org/gazebo/ubuntu-stable focal/main amd64 libignition-plugin amd64 1.4.0-1~focal [42.4 kB]
Fetched 199 kB in 1s (286 kB/s)
debconf: delaying package configuration, since apt-utils is not installed
(Reading database ... 156570 files and directories currently installed.)
Preparing to unpack .../libignition-plugin-dev_1.4.0-1~focal_amd64.deb ...
Unpacking libignition-plugin-dev:amd64 (1.4.0-1~focal) over (1.3.0-1~focal) ...
Preparing to unpack .../libignition-plugin_1.4.0-1~focal_amd64.deb ...
Unpacking libignition-plugin:amd64 (1.4.0-1~focal) over (1.3.0-1~focal) ...
Setting up libignition-plugin:amd64 (1.4.0-1~focal) ...
Setting up libignition-plugin-dev:amd64 (1.4.0-1~focal) ...
Processing triggers for libc-bin (2.31-0ubuntu9.9) ...

I need 1.4.0 for the namespace update https://github.com/gazebosim/gz-plugin/tree/ignition-plugin_1.4.0/loader/include

As discussed in Daily 2023.05.26, let's drop syncing with 6.
(See task)


IGNITE-12193 [Phase-1] Rebalancing cache group keys metric counters

@Mmuzaf please check if I understood you correctly? I would like to discuss the description and documentation of the properties.

This PR is associated with a resolved ticket. Closing the PR, please reopen it if you still need it.


Seperate models,views,serializers

I have seperated each class into one file.

Good job! Let's merge.

Nice job! The code is now well-organized. Let's merge this.

Nice, Let's merge.


Menu title overlaps content when side bar is collapsed

@mcflyph Can you please provide more context including some screenshots to explain the UX Fail?

Found a way to fix this:
const useStyles = makeStyles(
  (theme) => ({
    //....
    open: {
      width: 200,
    },
    closed: {
      width: 55,
    },
  }),
  { name: "RaTreeMenu" }
);

//...
//...

 return (
    <div
      className={classnames(classes.main, {
        [classes.open]: open,
        [classes.closed]: !open,
      })}
      {...rest}
    >
      {hasDashboard && (
        <DashboardMenuItem
          onClick={onMenuClick}
          dense={dense}
          sidebarIsOpen={open}
        />
      )}
      {resRenderGroup}
    </div>
  );

Also waiting! Thanks @Th1nhNg0


updating gas limits and fp contracts

Adding a New Chain
This PR update Base Mainnet and Sepolia in the registry. It updates the gas limits to the latest values pulled from the SystemConfig contracts. It also adds Base Mainnet's newly deployed fault proof contracts.
Checklist

[ ] I have declared the chain at the appropriate Superchain Level.
[x] I have run just validate <chain-id> locally to ensure all local validation checks pass.
[x] I have run just codegen to ensure that the chainlist and other generated files are up-to-date and include my chain.

@mds1 and @bitwiseguy the gas limit questions are good to get to the bottom of so I recorded this issue and pulled them out of this PR because its more important to get these fault proof addresses documented.


Validate request error message

I'm not displaying a full progress bar, instead I'm showing a small drop area for the file upload. If there is an error it won't show since the area is so small. How can I catch the error? I want to be able to use an alert or growl style message to show the user the error.

Hi
I have the same question: is there callback on server error? The finished callback is only called on success.
Mark

There is no callback per se. But you do receive error back on client.

Thanks @tomitrescak
But how can I access that error message? I see the error message returned in the xhr response. But I can't see a way to access that response (when using dropzone).
All tips greatly appreciated.
Thanks


in parameters, dynamic parameters and arguments

See https://github.com/dotnet/csharpstandard/pull/795/files#r1215631063
The interaction between dynamic (as an argument, or a parameter) and in parameter or in argument is not specified correctly.
There are several cases to cover:

in dynamic as parameter

in dynamic as argument
dynamic as argument
other reference type as argument
other value type as argument

other type as parameter

in dynamic as argument
dynamic as argument.

The following program answers most of the questions for the single parameter cases:
using System;

internal class Program
{
    private static void Main(string[] args)
    {
        dynamic d = 5;
        object o = new object();
        int j = 42;

        var c = new C();

        c.M(in d);
        // c.M(d); // Runtime binder exception (best overload has invalid arguments)
        c.M(in o);
        c.M(o);
        // c.M(in j); // CS1503: Cannot convert from 'in int' to 'in dynamic'
        c.M(j);

        c.M2(in j);
        c.M2(j);
        // c.M2(in d); // CS1503: Cannot convert from 'in dynamic' to 'in int'
        // c.M2(d); // Runtime binder exception (best overload has invalid arguments)
        // c.M2(in o); // CS1503: Cannot convert from 'in object' to 'in int'
        // c.M2(o); // CS1503: Cannot convert from 'object' to 'in int'

        c.M3(in d);
        // c.M3(d); // Runtime binder exception (best overload has invalid arguments)
        c.M3(in o);
        c.M3(o);
        // c.M3(in j); // CS1503: Cannot convert from 'in int' to 'in object'
        c.M3(j);
    }
}

public class C
{
    public void M(in dynamic p) => Console.WriteLine(p);
    public void M2(in int p) => Console.WriteLine(p);
    public void M3(in object p) => Console.WriteLine(p);
}

The interaction between dynamic (as an argument, or a parameter) and in parameter or in argument is not specified correctly.

In the case in is specified it will have behavior similar to ref, outside that it is not supported.

In the case in is specified it will have behavior similar to ref, outside that it is not supported.

For the spec, should we use the language "the behavior is undefined"? So, from the examples above, anywhere the method is void M(in dynamic d), and the argument doesn't have the in modifier, the result is undefined?


Record number of skipped executions during aggregation

Description
Corresponding PRs:

https://github.com/dockstore/dockstore/pull/5822
https://github.com/dockstore/dockstore-ui2/pull/1938
https://github.com/dockstore/compose_setup/pull/251

This PR modifies the metrics aggregator so that it aggregates workflow executions even when there are invalid executions (it previously skipped aggregating the metric entirely) and records the number of executions that were skipped. It validates executions using the validation constraints defined in the webservice metrics objects.
I did some minor refactoring that involved changing the interfaces to abstract classes to remove unnecessary task methods from the Validation Status aggregator which don't have the concept of task metrics.
Review Instructions
See https://github.com/dockstore/dockstore-ui2/pull/1938
Issue
SEAB-6046
Security
If there are any concerns that require extra attention from the security team, highlight them here.
Please make sure that you've checked the following before submitting your pull request. Thanks!

[x] Check that you pass the basic style checks and unit tests by running mvn clean install in the project that you have modified (until https://ucsc-cgl.atlassian.net/browse/SEAB-5300 adds multi-module support properly)
[x] Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
[x] If you are changing dependencies, check with dependabot to ensure you are not introducing new high/critical vulnerabilities
[x] If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.

(build will fail until I update the webservice version to a tag)


Fix Ci/CD

CI/CD is failing on dbt seed for no obvious reason. Runs locally fine with the same credentials 🤦♂

@clrcrl am I missing anything obvious? This integration_testing pattern is new to me.

Removing --warn-error in our CI script as a last resort. Seems to be a bug in dbt as not combinations of seed configs worked:
seeds:
  +quote_columns: false

seeds:
  artifacts_integration_tests:
    +quote_columns: false

seeds:
  artifacts_integration_tests:
  +quote_columns: false


Export-Csv should have option to suppress quotes

Export-Csv should have option to suppress quotes
By default Export-Csv puts quotes around all fields.   This isn't always necessary and some programs even have problems reading the results.  For example, Export-Csv will quote a numbers and dates:
…,"227","6/1/2018 12:00:00 AM"
Some programs then interpret the data as strings, which generally isn't the intent.
Also, quotes generally aren't required for strings (unless they contain the delimiter character).
Not using quotes (unless needed) would improve usability with other applications and reduce file output size.
Proposed technical implementation details
A couple options:

An option -UseQuotes which could be Never, Always, or (optionally) AsNeeded, StringsOnly.
An option to specify the quote character/string, then I could do -Quote "" to suppress quotes.
An option -QuoteFields  to specify which fields to quote.

Any of these would solve my current problem (all of them would be a complete, powerful solution to most problems).
-UseQuotes options details:

Never - don't quote anything, ever.
Always - quote everything (current behavior).
AsNeeded - only quote fields that need it (they contain a delimiter character).
StringsOnly - quote all strings, but not numbers, dates, or other non-string values.

There's a good argument to be made that the default behavior should be AsNeeded or StringsOnly as they tend to be more compatible with other applications; however, it would be different and could break existing usage.

Imo, an AsNeeded should be the default, despite the fact that it would probably be a bit of a break in behaviour from what it was previously.

Yep.  Now that I think about it, another option would be StringsOnly (so it would quote strings, but not numbers or dates).  I'll add it to the proposal.

AsNeeded should be the default

It is a breaking change. It's unclear what area it is.

Thanks for the fast action!  FYI, a use case for StringsOnly is where numeric data needs to be treated as a string.  Presumably the information in already stored in PowerShell as a string, so using StringOnly would force PowerShell to quote the string so the program reading the data knows it's a string and not numeric.  (Unfortunately, Excel is dumb in this regard and treats quoted numerics as numerics, not strings; however, there are other programs that don't.)
An example would be ID#s which may have leading zeros (e.g. "0004738749837"), especially if the leading zeros are significant (e.g. the data contains both "001234" and "0000001234").  Such situations can arise when using variable length coding schemes.
Another case would be hex data.  Some programs will look at 0087 and treat is as numeric, even though further down in the same column we might have 09DA, which is clearly intended to be hex.  Forcing the data to be quoted would (ideally) treat the entire column as strings, not just those that have letters in them.
Consider the case of encoding a binary value into a string.  "0012" might represent two bytes coded as a string, while "12" (leading zeros stripped) would represent a single byte.

@DavidBerg-MSFT I think what @iSazonov is concerned about is that when translating some types of objects to a string form, you can end up with their representation containing commas. In fact, some systems of recording numbers will result in commas depending on the culture settings. I believe several European cultures actually notate a decimal point with a comma instead of a period. This would break the resulting file, if you attempted to have it create the file without quoting numbers as strings.
Ultimately, trying to determine ahead of time which types of objects can and cannot stringify well and not break the format is liable to be a very large waste of time; the file created is going to be a text file, by design of the CSV format. It is arguably much more effective to simply assume data is string and allow the program that intends to consume it to determine the format however is appropriate for its requirements.

@vexx32. Yes, in many countries, a number that we would write in the US as 1,234.56 would be represented as 1.234,56.  In the US, default numeric formatting usually drops the comma (e.g. 1234.56); however, in the other countries it's the period that would be dropped (e.g. 1234,56), so we still have to deal with the comma.
In those cases (numbers that contain a comma) it would be necessary to either:

Use Always (current default),
Use AsNeeded, or
Use a different delimiter (e.g. a tab or semi-colon).  Then we can use Never or StringOnly (which would otherwise break).

That's a strong argument for making Always or AsNeeded the default, but I don't think it eliminates the value of Never and StringOnly (especially since we can change the delimiter, I particularly like tab as it almost never occurs in string values).
I've run into the same problem at times importing data where unquoted text contains commas.  In those cases, if I can't quote the text, I usually try replacing the delimiter with a non-conflicting one.
Yes, ideally the target program will handle the data correctly; however, the more/better hints we can give it, the easier it is to work with the data.

I think I can agree with all of that! 😊
We should document those caveats with Never and/or StringOnly options to avoid confusion, though. I'm sure some users will still run into it, but as long as it's clearly documented I don't see an issue with it. It's essentially an opt-in behaviour to relax aggressiveness of quoting values based on criteria or just not use quotes at all, so I think it makes sense to allow it and document the potential pitfalls with suggestions of how to avoid them. 😄

@DavidBerg-MSFT Should StringOnly include AsNeeded behavior? Or maybe we need still StringOnlyAndAsNeeded option?

My assumption is that StringOnly would be Always for just Strings.  I can't think of a use case where I would want StringOnly+AsNeeded that wouldn't also be satisfied by AsNeeded.

@DavidBerg-MSFT I am trying to add StringOnly and catch a problem. I can discover that original type of value is System.String. It is simple and it works. But there is Path type. Example: gps pwsh | ConvertTo-Csv -UseQuotes StringOnly. I'd expect that Path should be quoted. This complicates the solution - it is not clear what types we should quote.

@isazonov since all values in a CSV are basically going to convert to string data in the file itself, maybe you could do a simple check on if (value.ToString().Contains(' ')) to determine if it needs quotes and is not itself a string value?

@vexx32 This is not necessary as long as there is no conflict with delimiter that already implemented as AsNeeded option. My question is that there are types that we usually see as strings. And I want to get confirmation that we ignore this and quote only values of string type.

Yes, when I was thinking of -StringOnly, I admit I was only thinking about fundamental data types (strings, numbers, dates, Booleans); however, any object which is not one of those fundamental data types is going to get converted to a string by calling it's ToString() method.  Which brings up a question on how to treat non-fundamental types.   I see a couple options:

Always treat non-fundamental types as non-strings.  Easiest: we test for string type, anything else isn't quoted).   And most literal (StringsOnly does exactly what it says).  Fallback is to use AsNeeded.
Always treat non-fundamental types as strings (tougher, because then StringsOnly really becomes DontQuoteNumbersDatesAndBooleans…., requiring more testing, and becoming more confusing about what is or isn't considered a string).
Something like option 1, but also check for something like a ShouldOutputAsString field/property on objects... (too complex, I wouldn't do it just for this; however, I have built systems where data objects had metadata properties that provided hints for how to properly format them in different situations, it was quite nice).
Eliminate StringsOnly in favor of -QuoteFields , as that is a superset (allowing the caller to specify what they do/don't want quoted).
Eliminate StringsOnly as not being well enough thought out.

In terms of preference, I'd probably pick (in order) 4, 1, 5 (and not really consider 2 or 3).  What do you think?  @iSazonov?
Thanks for all the effort!

@DavidBerg-MSFT Thanks for feedback!
I'd prefer that we do "smart things". This means that users get "expected" results. Question is that is "smart" and "expected" in the context.
As @vexx32 pointed all is converted to strings before output to CSV. So user expectations can be based only on original types of values. We could implement exclude and include type lists with common defaults. In the case it is not clear benefits because excluding only fundamental types doesn't look smart enough. Also we'll have to add parameters to allow users modify the lists. Fallback to AsNeeded can confuse users too - it can be "over-smart". This leads us to believe that StringOnly parameter is not as useful as expected and QuoteFields is better alternative - does not require additional parameters and it is very fast.


Revert "Add tag for service"

Per discussion on #131.
Reverts opentracing/specification#119
We can add a better replacement in a separate PR, but I wanted to get this removed before it gets too wide spread.

I think we've gotten the feedback we needed.  Merging to avoid further confusion.


Don't break goto search result and goto error for other languages

I finally figured out an issue that has been driving me crazy for months. After some time going to a match from e.g. a *grep* or *Compile-Log* buffers just stopped working even though the relative file-name of the search result as well as the displayed and effective default-directory of the buffer are correct.
Turns out it stops working once rustic has been loaded and has established its advice to forcefully let-binds default-directory to the root of the current project. That's nasty. It might be the correct thing to do for rust. For many many other things it obviously is not.
I recommend you give up on "fixing" the default-directory after the fact. If cargo spits out paths that are relative to the root of the project even when invoked from a subdirectory, then adjust the default-directory before calling cargo. e.g. by doing the let-binding around that call and nothing else.

It might be the correct thing to do for rust.

#110 suggests that it might not even be appropriate here.

Yeah, sorry about that. I remember that I wanted to fix this....
There's another hook that has to be fixed...

I just removed it for now.

Yey!  I've been trying to track down why grep breaks for me and just today I found rustic installing a hook that messes with default-directly.
Glad to see this fixed!  Thanks.

I really need somebody reviewing my PRs =)


Some nodes lose reference in the settings data file

Reproducing this issue has been difficult - it seems random. Not all of the ObjectField's lose reference - just some.

Haven't been able to reproduce this


Workspace.Windows collection is not accessed/updated atomically

I often get the following exception while running workspacer:

We either need to provide locked access on object, or make updates atomic.
We should probably also replace IEnumerable<> with an explicit Array-type or List-type, to be better able to control enumeration and avoid situations like this.
Unless someone else beats me to it, I will probably try to debug this more in depth and find out where the collisions occur, and possibly tighten up the code.

Yep, workspacer does a lot of things non-atomically all over the place, and mutates global state on a bunch of threads.
This is one of the places where the initial design failed to fulfill the requirements of the full application, so things got a little hairy, although it doesn't cause problems usually.
I'm open to suggestions as to how to improve this, short of gutting the entire application in a refactor (not opposed, but would enjoy some direction first).

Ok. So based on the branch I've submitted as "Managed Windows", I've been able to reproduce this error, and I think fixing it is pretty straight forward.
I'll update the PR to include those fixes, and then merging that PR should also close this bug.
So I'm working with Windows, and then VS breaks workspacer into Debug mode:

Looking into parallel stacks there are only 2 concurrent stacks working with Windows:

What's going on in the first thread is pretty obvious: We're enumerating _windows and it's being modified underneath our feet.
Looking at the other stack, I just went down a few frames until I found this:

And currentWorkspace here is the same workspace as the one producing the exception! (Notice it's the same object, with object-id $1).
So basically what we need to do is add the following code around AddWindow(), RemoveWindow() and the ManagedWindows-property:
lock (_windows)
{
   // code
}

And this problem should be history.
Any objections?


Addressed comments from original Foreman Provider explorer add PR.

Follow up PR to address comments in https://github.com/ManageIQ/manageiq/pull/1981 from @gmcculloug

Configuration button is added when individual Foreman Provider node is selected in tree, so Refresh/Edit/Remove actions can be performed for individual Foreman Provider.
Added code to refresh a Foreman Provider after it's added.
Reload tree when "reload" button is pressed, that will remove any Providers that were queued up for deletion and have been removed from db.

@gmcculloug @dclarizio please review/test

@h-kataria Can you also fix https://github.com/ManageIQ/manageiq/pull/1981#discussion-diff-25735192 and  https://github.com/ManageIQ/manageiq/pull/1981#discussion_r25735249

@h-kataria Selecting an individual provider shows the configuration button and I can refresh and edit it.  However, when I select "Remove this Foreman Provider from the VMDB" I get a popup box to confirm the deletion and press "OK" and then see the flash error message "No Providers were selected for deletion"
Also I am still not seeing a refresh initiated after adding the provider.

@gmcculloug fixed reported issues.

With the last update I now get the flash error message "Error during 'refresh_ems': underined method 'base_model' for NilClass:Class" when adding a new Foreman provider.  I am not doing anything special so should be very easy to reproduce.

@gmcculloug fixed that, had to pass in foreman_provider.configuration_manager.id instead of foreman provider.id to Ems Refresh call.

@h-kataria Retested with latest changes and that is working now.  Thx


Cannot mount folder from inside node_modules/.cache folder

Original Discussion: https://github.com/pikapkg/snowpack/discussions/1267
/cc @bluebrown
This isn't a primary use-case, but if it's easy to support, we should. This needs to be debugged a bit more to see what's going on. See original discussion for more reproduction info:
  "mount": {
    "node_modules/.cache/11ty": "/",
    "src/assets": "/static"
  },

Hi Fred,
I found out that node_modules is by default ignored based on this code snippet and below piece of documenation
builds.ts line 382
  // 0. Find all source files.
  for (const [mountedDir, mountEntry] of Object.entries(config.mount)) {
    const allFiles = glob.sync(`**/*`, {
      ignore: [...config.exclude, ...config.testOptions.files],
      cwd: mountedDir,
      absolute: true,
      nodir: true,
      dot: true,
    });
    for (const rawLocOnDisk of allFiles) {
      const fileLoc = path.resolve(rawLocOnDisk); // this is necessary since glob.sync() returns paths with / on windows.  path.resolve() will switch them to the native path separator.
      const finalUrl = getUrlForFileMount({fileLoc, mountKey: mountedDir, mountEntry, config})!;
      const finalDestLoc = path.join(buildDirectoryLoc, finalUrl);
      const outDir = path.dirname(finalDestLoc);
      const buildPipelineFile = new FileBuilder({filepath: fileLoc, outDir, config});
      buildPipelineFiles[fileLoc] = buildPipelineFile;
    }
  }

Docs
exclude | string[]
Exclude any files from scanning, building, etc. Defaults to exclude common test file locations: ['**/node_modules/**/*', '**/__tests__/*', '**/*.@(spec|test).@(js|mjs)']
Useful for excluding tests and other unnecessary files from the final build. Supports glob pattern matching.

Based on this my idea was to use an empty array for the exclude key but it doesnt work
snowpack.json
  "mount": {
    "node_modules/.cache/_11ty": "/",
  },
  "exclude": [],

So I check the config.ts file and I found this
config.ts line 34
const ALWAYS_EXCLUDE = ['**/node_modules/**/*', '**/web_modules/**/*', '**/.types/**/*'];

So if one wanted to solve to very simple, the node_modules could be moved to the default config instead of this "private" exclude array like shown below. However, this would mean the user could accidentally unset this easily.

const ALWAYS_EXCLUDE = ['**/web_modules/**/*', '**/.types/**/*']; // move from here

// default settings
const DEFAULT_CONFIG: Partial<SnowpackConfig> = {
  plugins: [],
  alias: {},
  scripts: {},
  exclude: ['**/node_modules/**/*'], // to here
  installOptions: {},
  devOptions: {
    secure: false,
    hostname: 'localhost',
    port: 8080,
    open: 'default',
    out: 'build',
    output: 'dashboard',
    fallback: 'index.html',
    hmrDelay: 0,
    hmrPort: 12321,
    hmrErrorOverlay: true,
  },
  buildOptions: {
    baseUrl: '/',
    webModulesUrl: '/web_modules',
    clean: false,
    metaDir: '__snowpack__',
    minify: false,
    sourceMaps: false,
    watch: false,
  },
  testOptions: {
    files: ['__tests__/**/*', '**/*.@(spec|test).*'],
  },
  experiments: {
    ssr: false,
  },
};

While Investigating I was thinking of something like and include array as well. Items in this array could overwrite any excluded item which would be more solid for users who don't really know how it works. I imagine something like this.
"mount": {
    "node_modules/.cache/_11ty": "/",
},
"exclude": [
    "something/to/exclude/without/unsetting/node_modules"
],
"include" [
    "path/to/overwrite",
    "node_modules/.cache/.1tty"
]

What do you think? I would be happy to tackle either approach or something different and make a pr.

Oh of course! Great catch, that's exactly what's happening.
As a workaround, I wonder if adding this to your config would solve the problem:
// "!" will tell our glob library to do the opposite
"exclude": [
    "!node_modules/.cache/_11ty/**"
],

Hi Fred,
that is actually one thing that I have tried but the result was not what I expected. On second thought it makes sense though.
This will tell now snowpack to ignore everything that's not exactly named "!node_modules/.cache/_11ty/**". That's the whole project.
The result is that everything will be ignored, including the node_modules/.cache directory, since this is still ignored from the ALWAYS_EXCLUDE  array,
So in the build is only an env.js and the __snowpack__ folder but nothing else with this setting.

What about "**/node_modules/.cache/_11ty/**/*"? It should work in the same array as ALWAYS_EXCLUDE, so maybe it just needs to be fiddled with a bit?

You are right, it should technically work, looking at the glob package.
First the excludes are merged.
config.ts line 567
  config.exclude = Array.from(
    new Set([...ALWAYS_EXCLUDE, `${config.devOptions.out}/**/*`, ...config.exclude]),
  );

and then used as argument for glob.sync
  for (const [mountedDir, mountEntry] of Object.entries(config.mount)) {
    const allFiles = glob.sync(`**/*`, {
      ignore: [...config.exclude, ...config.testOptions.files],
      cwd: mountedDir,
      absolute: true,
      nodir: true,
      dot: true,
    });

But when I try to use this in the snowpack config, it doesn't work for some reason. I have created this sample repl that shows that it should work, if this would be all there is to it. So the fact that it doesn't work means that there is something else that I haven't found yet, I will need to look further into the source code.
const glob = require('glob');

const ALWAYS_EXCLUDE = ['**/node_modules/**/*'];

const exclude = ["!**/node_modules/glob/**/*"];

const toSync = Array.from(new Set([...ALWAYS_EXCLUDE, ...exclude]));

const allFiles = glob.sync(`**/*`, {
  ignore: exclude,
  cwd: "./",
  absolute: true,
  nodir: true,
  dot: true,
});

console.log(allFiles);
/* output:
[
  '/home/runner/AdoredSingleSmalltalk/node_modules/glob/changelog.md',
  '/home/runner/AdoredSingleSmalltalk/node_modules/glob/common.js',
  '/home/runner/AdoredSingleSmalltalk/node_modules/glob/glob.js',
  '/home/runner/AdoredSingleSmalltalk/node_modules/glob/LICENSE',
  '/home/runner/AdoredSingleSmalltalk/node_modules/glob/package.json',
  '/home/runner/AdoredSingleSmalltalk/node_modules/glob/README.md',
  '/home/runner/AdoredSingleSmalltalk/node_modules/glob/sync.js'
]
*/

I think it's not possible to do that with the glob package. See this repl: https://repl.it/talk/share/Glob-Test/57653
I have replicated roughly what is happening with the snowpack config. Below shows the actual code from snowpack.
config.ts line 567
  config.exclude = Array.from(
    new Set([...ALWAYS_EXCLUDE, `${config.devOptions.out}/**/*`, ...config.exclude]),
  );

builds.ts line 382
  for (const [mountedDir, mountEntry] of Object.entries(config.mount)) {
    const allFiles = glob.sync(`**/*`, {
      ignore: [...config.exclude, ...config.testOptions.files],
      cwd: mountedDir,
      absolute: true,
      nodir: true,
      dot: true,
    });

btw, in case it's of interest, i would like to mount general things in node_modules (in particular, fonts and images from lightgallery).  npx snowpack build ignores these mounts, though npx snowpack dev seems to accept them.  (should the behavior be consistent between the two?)

This issue has to get an high priority. Using CSS files from node_modules folder (with imports like images, icons, fonts) is a pain. I spent about 3 hours to figure it out (https://stackoverflow.com/questions/63871880/snowpack-woff2-font-wont-load/65545991#65545991).
Please fix!

This issue has to get an high priority. Using CSS files from node_modules folder (with imports like images, icons, fonts) is a pain. I spent about 3 hours to figure it out (https://stackoverflow.com/questions/63871880/snowpack-woff2-font-wont-load/65545991#65545991).
Please fix!

one interim patch might be to detect situations where the user appears to be trying to do the impossible, and emit a warning or error message, pointing them in the right direction?  (to be clear, somehow allowing this to "just work" would still be, imo, the best.)
cheers.

one interim patch might be to detect situations where the user appears to be trying to do the impossible, and emit a warning or error message, pointing them in the right direction?  (to be clear, somehow allowing this to "just work" would still be, imo, the best.)
cheers.

@natemoo-re do you know if this is covered by your most recent PR?

Quite sure that it is, but I have not tested a reproduction of the original issue to confirm.
The corrected, current behavior is to ignore anything within node_modules unless it has been explicitly included in mount.

I am able to mount a folder inside of node_modules, but it overwrites the built application.  For the config below, the single page application in app is either not written or it's overwritten.  The result of calling snowpack build is that only shoelace/icons/*.svg paths exist inside of the dist folder.
// snowpack.config.js
module.exports = {
  root: './app',
  buildOptions: {
    out: './dist/'
  },
  mount: {
    "node_modules/@shoelace-style/shoelace/dist/assets": { url: "/shoelace", static: true }
  },
}

<EMAIL_ADDRESS><EMAIL_ADDRESS>


Thumbnail gen

Wip for thumbnail generation :)

added feature for #137

added feature for #132

removed language picker menu for #84


Enable sonar

Coverage remained the same at 82.962% when pulling 389ffd8b7061f0898c3a14fbbc5c2c03648f3c07 on enable-sonar into 6885f206a2111e571d1522668dc0c215c6c454ed on master.


Handle zero-length OK deflate responses (master)

What is the purpose of this pull request?

[x] Bug fix

What changes did you make?
This fixes an issue where older ISS servers (Definitely 7.5, but I can’t rule out other versions), could respond with a zero-length OK deflate response, hanging node-fetch. This was caused by the body stream never emitting a data event. A typical problematic response looks something like this:
HTTP/1.1 200 OK
Content-Encoding: deflate
Content-Length: 0
Server: Microsoft-IIS/7.5
X-AspNet-Version: 4.0.30319
X-Powered-By: ASP.NET

Is there anything you'd like reviewers to know?
This is the master counterpart to the 2.x PR #903
I had to tweak it a bit compared to the 2.x version because pump would incorrectly reject on empty responses.

fix #1442
fix #922
fix #903

Not really sure how to fix the coverage here. In the original PR adding pump (#670), @xxczaki mentions that the stream error conditions are hard to test.

HTTP/1.1 200 OK
Content-Encoding: deflate
Content-Length: 0
Server: Microsoft-IIS/7.5
X-AspNet-Version: 4.0.30319
X-Powered-By: ASP.NET is change first and merge is accept

Don't see what you're saying here, looks like it might be Hacktoberfest spam?

Ping @jimmywarting as it’s been a couple weeks. This addresses your comment https://github.com/node-fetch/node-fetch/pull/903#issuecomment-690946225

@mgoszcz2 Is it ready for review now?

I think so @xxczaki. No bother at all, you can see I was in no rush either.

Hi, any chance of this getting merged? @xxczaki @bitinn

@mgoszcz2 This just needs to be rebased on main.  I can approve once that's done, and I believe @bitinn just needs to re-review after that and we can merge.

@tekwiz Thanks. I hope this also means #903 will be merged? Because there was a lot of churn involved in getting to this stage without any merges happening and I don't feel like sinking more time into this pull request. I made #903 to fix a specific issues that was affecting production at a company I no longer work. Unless nothing more is required of me other than a rebase, I'm not willing to continue working on this PR.

I believe this is addressing the same issue as
https://github.com/node-fetch/node-fetch/issues/1442
I hope Jeff can help to look into it. We are now 2 years away from my initial review, I am happy to merge but we need to make sure it doesn’t introduce any problems to current codebase.
Sent from my iPhone

On Jan 16, 2022, at 21:01, Maciej Goszczycki @.***> wrote:

@tekwiz Thanks. I hope this also means #903 will be merged? Because there was a lot of churn involved in getting to this stage without any merges happening and I don't feel like sinking more time into this pull request. I made #903 to fix a specific issues that was affecting production at a company I no longer work. Unless nothing more is required of me other than a rebase, I'm not willing to continue working on this PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you were mentioned.

Can you explain what's preventing from #903 from being merged? It's a completely different branch and major version. I wasn't even using 3.x, I only made this PR because I was asked to by Jimmy.

I rebased it

Regarding, #1442: Yes it's the same issue, and in fact that it's a duplicate of #922 I opened in 2020. In case it's not clear, this PR would've fixed it on the main branch, and #903 would've fixed it on the 2.x branch (but nether has been merged).

Thx, for 2.x branch backport we can reopen your PR, please ping Jimmy in the comment. I think a lack of active reviewers at the time was preventing it from moving forward.
Sent from my iPhone

On Jan 16, 2022, at 23:24, Maciej Goszczycki @.***> wrote:

I rebased it
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you were mentioned.

Done. Please also consider merging/re-opening #903 when you have some time.

@mgoszcz2

Done. Please also consider merging/re-opening #903 when you have some time.

haven't looked trough (reviewed) it yet... it's closed... what dose that pr do that this pr dose not solve?

It fixes this bug for the 2.x branch. You closed it. I do not understand why. When I asked if it will be merged and you said it will be. That was September 2020. I only made this PR because you asked for it to be ported to the main branch too. Please look over your own messages in #903. In retrospect I should have never offered to fix the main branch if I knew merging 6 lines of code in two branches would take nearly two years.

I pinged you at the request of David. Please read the messages above.

Thx, for 2.x branch backport we can reopen your PR, please ping Jimmy in the comment. I think a lack of active reviewers at the time was preventing it from moving forward.

merged, thx for the fix, again sry that it took so long


Version displayed on startup is not correct

2021-11-11 22:26:32  INFO Running version: 0.1.1

I am fairly certain I am running a version that is newer than the 0.1.1 tag (I pulled the preview docker tag) but this is not reflected in the version number.

As discussed, our use of the preview tag might interact with this badly.
Let's get rid of the preview tag and instead publish a new docker container every time we push to master.

If I understand you correctly: you want to get rid of the preview tag and release in favor of having just docker images?

If I understand you correctly: you want to get rid of the preview tag and release in favor of having just docker images?

Yes. I am not sure if it solves the above problem but our use of the preview tag interacts badly with vergen anyway because it will just say preview which gives us no indication of which version it is.

my understanding is that it shouldn't though... we are using git lighttags and VERGEN_GIT_SEMVER_LIGHTWEIGHT should use that.
So the question is, why does vergen not use it? If I use a local tag  and build it, it works.
Anyway, instead of removing the preview release, wouldn't it be better to use a different var, e.g.
VERGEN_GIT_SEMVER_LIGHTWEIGHT-VERGEN_GIT_SHA_SHORT
this would print 0.1.1-75-gbe4b8fa-be4b8fa on the latest master.
or with a test tag: test-be4b8fa

Anyway, instead of removing the preview release, wouldn't it be better to use a different var, e.g. VERGEN_GIT_SEMVER_LIGHTWEIGHT-VERGEN_GIT_SHA_SHORT
this would print 0.1.1-75-gbe4b8fa-be4b8fa on the latest master. or with a test tag: test-be4b8fa

I am not a fan of taping over problems. Our use of vergen conflicts with the idea of a rolling Git tag. It might not be directly related to this issue but if we fix this issue, it would surface that our use of vergen is useless. Hence I want to do both:

Fix this issue so vergen picks up the correct tag
Get rid of the preview tag because it will conflict with that solution

Are you against using VERGEN_GIT_SEMVER_LIGHTWEIGHT-VERGEN_GIT_SHA_SHORT?

Looking into the issue why we don't pick up the tag: the issue could be how we checkout our repository

```
  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +refs/heads/master*:refs/remotes/origin/master* +refs/tags/master*:refs/tags/master*
```

taken from https://github.com/itchysats/itchysats/runs/4220905293?check_suite_focus=true

As you can see, we do not fetch the tags, so that could be one issue.

The other one is that we checkout `github.event.release.target_commitish` which should resolve to the branch.
But what we actually want is checkout the tag, this should be in `GITHUB_REF`. I'll give this a try on a fork

Are you against using VERGEN_GIT_SEMVER_LIGHTWEIGHT-VERGEN_GIT_SHA_SHORT?

No, I don't mind which vergen strategy we use as long as it uniquely identifies the commit the software has been built of.
Like I said above, it is not directly related but I still want to get rid of the preview tag. We've changed our assumptions from when we introduced that. We are using docker images now which can also be published directly off master so we no longer need to do this hack of having a rolling tag to ship artifacts.

Ah, now I understand you: so you want to get rid of the whole preview release workflow: including binaries and tag and just publish docker whenever we merge into master.
That would simplify things but we lose the binaries.

That would simplify things but we lose the binaries.

I don't think we need them for every merge to master, do we? Normal releases will have binaries and everyone who wants the new hot stuff can use docker.

I double down on the assumption that we do not pull the tags correctly:

we fetch the repository without tags

  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +refs/heads/master*:refs/remotes/origin/master* +refs/tags/master*:refs/tags/master*

--no-tags

    By default, tags that point at objects that are downloaded from the remote repository are fetched and stored locally. This option disables this automatic tag following.

I reproduced this locally:
Fetch first
 git fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin

Checout master
git checkout --progress -B master refs/remotes/origin/master

Show tags
git log --no-walk --tags --pretty="%h %d %s" --decorate=full

be4b8fa  (tag: refs/tags/test) Merge #571
dc190cf  (tag: refs/tags/0.1.1) Merge #506
5891ad6  (tag: refs/tags/0.1.0, refs/heads/dont-fail) Merge #398
990b23a  (tag: refs/tags/preview) Merge #291

--> as you can see, preview is quite outdated
So I force fetch it, preview gets updated
 git fetch --tag --force

From github.com:itchysats/itchysats
 t [tag update]      preview    -> preview

Show log again
git log --no-walk --tags --pretty="%h %d %s" --decorate=full

0fc8b24  (grafted, HEAD -> refs/heads/master, tag: refs/tags/preview, refs/remotes/origin/staging, refs/remotes/origin/master, refs/remotes/origin/HEAD) Merge #699
be4b8fa  (tag: refs/tags/test) Merge #571
dc190cf  (tag: refs/tags/0.1.1) Merge #506
5891ad6  (tag: refs/tags/0.1.0, refs/heads/dont-fail) Merge #398

That being said, I think the bug here can be fixed by telling the github action to pull tags.

In regards to the other point:

I don't think we need them for every merge to master, do we? Normal releases will have binaries and everyone who wants the new hot stuff can use docker.

A simple solution would be to remove the logic around creating preview builds and trigger a github action which builds the docker image. For that I would propose to build the taker and maker inside the docker image.
This might be slower, but would give us reproducable docker images and we can make use of the power of buildx to create multiplatform docker images, e.g. something like:

(untested)

FROM debian:bullseye-slim
LABEL org.opencontainers.image.source https://github.com/itchysats/itchysats
ARG GIT_TAG
ARG FRONTEND
ARG BINARY
RUN git clone -b $GIT_TAG --depth 1 git@github.com:itchysats/itchysats.git /src/
RUN cd $FRONTEND-frontend && yarn && yarn build
RUN cargo build --release --bin $BINARY
VOLUME data
HTTP Port and P2P Port
EXPOSE 8000 9999
ENTRYPOINT ["/src/release/${BINARY}", "--data-dir=/data", "--http-address=<IP_ADDRESS>:8000"]

What do you think @thomaseizinger ?

Thanks for getting to the bottom of the tag issue!
Building within the docker image sounds good to me. Just need to make sure we use multi-stage builds to keep the image size small!
If it is not too hard, it would be amazing to use https://github.com/GoogleContainerTools/distroless/blob/main/base/README.md as the base :)

Re-opening this because now we are running into the following:
❯ docker run --pull always --rm -v$PWD/data/preview-taker:/data -p8886:8000 ghcr.io/itchysats/itchysats/taker --maker=<IP_ADDRESS>:9999 --maker-id=69a42aa90da8b065b9532b62bff940a3ba07dbbb
11d4482c7db83a7e049a9f1e testnet
latest: Pulling from itchysats/itchysats/taker
e8614d09b7be: Already exists
c6f4d1a13b69: Already exists
a1f1879bb7de: Already exists
7c94f3478345: Pull complete
Digest: sha256:2cdfccc2a3b608715d5991fcb7536f405046ef45600af2a234ae17b925cdedce
Status: Downloaded newer image for ghcr.io/itchysats/itchysats/taker:latest
2021-11-29 23:52:43  INFO Initialized logger
2021-11-29 23:52:43  INFO Running version: preview
2021-11-29 23:52:46 DEBUG Taker actor system ready