virt-handler, Add missing network name for SRIOV interfaces in case MAC is presented in the VMI spec

Since SRIOV interfaces are HostDev, they don't appear on domain.Spec.Devices.Interfaces
but just on domain.Status.Interfaces (based on guest agent report),
and on domain.Spec.Devices.HostDevices.
For non HostDev we have a mapping between the mac and the network name for each interface
that was declared in the spec.
For HostDev SRIOV, the VFIO device type
doesn't support getting the mac address at the current stage,
so we cannot create such mapping, hence we cannot match between the report of guest agent
MAC and the network name.
Add logic that in case the vmi.Spec.Domain.Devices.Interfaces
has a MAC address for the SRIOV interface, it would be used to find
the matching interface of domain.Status.Interfaces and report back the
network name according the vmi.Spec.Domain.Devices.Interfaces network name field.
Notes:

In case there isn't a matching MAC (if the mac wasn't set explicitly in the Spec), a name won't be reported.
In case there are two entries with the same MAC on vmi.Spec.Domain.Devices.Interfaces
which match the one reported by the guest agent on domain.Status.Interfaces,
one of them will be reported as network name.

SRIOV tests:
Validate that the network name is set in the VMI scheme,
and matched the expected interface.
2nd Phase would either include MAC generating in case the MAC isn't set in the VMI spec,
or reading the actual configured MAC, in case it would be supported for VFIO devices.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1856347
Note for reviewers
Signed-off-by: Or Shoval<EMAIL_ADDRESS>VMI status displays SRIOV interfaces with their network name only when they have originally
been defined with an explicit MAC address in the VMI spec.

/lgtm

Thanks
/retest

adding test

Added tests, and fixed the dependency of ethN interface order.

Rebased

/lgtm

Thanks Ram
@AlonaKaplan
can you review please?
once you approve, please add /hold as well, so we can test it manually by Ram,
as Edy asked that non author will test it as well, because the SRIOV lane is dead.

Thanks Ram
@AlonaKaplan
can you review please?
once you approve, please add /hold as well, so we can test it manually by Ram,
as Edy asked that non author will test it as well, because the SRIOV lane is dead.

/hold adding the hold now so I won't forget

I think the problem is that we are using <hostdev type='pci'> and not <interface type='hostdev'>. See reference.
If we will use <interface type='hostdev'> we will be able to easily pass the alias, mac address and vlan id. And the report from libvirt will match regular interfaces report.
@rmohr @vladikr do you know if there is any historical reason why we use hostdev and not an interface with hostdev type (btw, afaik in ovirt we use interface with type hostdev).

Since SRIOV interfaces don't appear on domain.Spec.Devices.Interfaces

That's because it is on domain.Spec.Devices.HostDevices.

but just on domain.Status.Interfaces (based on guest agent report),
the logic of adding them to the VMI spec is different, and the Alias.Name
is missing.
Add logic that in case the vmi.Spec.Domain.Devices.Interfaces
have a MAC address for the interface, it would be used to find
the right interface of domain.Status.Interfaces and report back the
name according the vmi.Spec.Domain.Devices.Interfaces name field.
In case there isn't a matching MAC, a name won't be reported.
In case there are two entries with the same MAC on vmi.Spec.Domain.Devices.Interfaces
which match the one reported by the guest agent on domain.Status.Interfaces
the first name will be reported.
Since we don't support bond on SRIOV yet we can't test it, nor it would be a problem yet.
When we do support it, we would be able to
check which other fields can be tested in order to select the right one
(as domain.Status.Interfaces doesnt report type, but, for example,sriov: {} exists on
vmi.Spec.Domain.Devices.Interfaces)

I prefer what I suggested on the comment.
But if there is a good reason for not using  then I would try to add the alias and the mac to the hostdev (I guess the mac would be ignored by libvirt, but it will allow us to match between the device and the status).
BTW, do you know why we do the match by the mac address? Isn't the alias reported back by libvirt and can be stored on the status?
Regarding the approach i this PR, I don't think it is good enough. It solves the bug in a partial way, just if the mac is specified in the spec, which is not always true. Do I miss something?

Tests: Added validation that network name is set in VMI scheme.
Use it also to find the interface name, in order to
not be depended on ethN naming which cant be depend on,
since it might be random each boot.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1856347
Signed-off-by: Or Shoval<EMAIL_ADDRESS>None

Just rebased, will check more about the approaches / wait for Roman and Vladik comments,
Removing the hold since the lgtm was removed as well, it was needed just in case there will be approve please.
/hold cancel

I think the problem is that we are using <hostdev type='pci'> and not <interface type='hostdev'>. See reference.
If we will use <interface type='hostdev'> we will be able to easily pass the alias, mac address and vlan id. And the report from libvirt will match regular interfaces report.
@rmohr @vladikr do you know if there is any historical reason why we use hostdev and not an interface with hostdev type (btw, afaik in ovirt we use interface with type hostdev).

Please see here.
<interface type='hostdev'> cannot be used because it requires access to the PF.

I think the problem is that we are using <hostdev type='pci'> and not <interface type='hostdev'>. See reference.
If we will use <interface type='hostdev'> we will be able to easily pass the alias, mac address and vlan id. And the report from libvirt will match regular interfaces report.
@rmohr @vladikr do you know if there is any historical reason why we use hostdev and not an interface with hostdev type (btw, afaik in ovirt we use interface with type hostdev).

Please see here.
<interface type='hostdev'> cannot be used because it requires access to the PF.

Thanks Eddy. Adding link to the "history" for the record.
@oshoval can you try the approach suggested in https://github.com/kubevirt/kubevirt/pull/4335#issuecomment-712448138 ?

I think the problem is that we are using <hostdev type='pci'> and not <interface type='hostdev'>. See reference.
If we will use <interface type='hostdev'> we will be able to easily pass the alias, mac address and vlan id. And the report from libvirt will match regular interfaces report.
@rmohr @vladikr do you know if there is any historical reason why we use hostdev and not an interface with hostdev type (btw, afaik in ovirt we use interface with type hostdev).

Please see here.
<interface type='hostdev'> cannot be used because it requires access to the PF.

Thanks Eddy. Adding link to the "history" for the record.
@oshoval can you try the approach suggested in #4335 (comment) ?

Already on it
https://github.com/kubevirt/kubevirt/pull/4335#issuecomment-712626990

Since SRIOV interfaces don't appear on domain.Spec.Devices.Interfaces

That's because it is on domain.Spec.Devices.HostDevices.

but just on domain.Status.Interfaces (based on guest agent report),
the logic of adding them to the VMI spec is different, and the Alias.Name
is missing.
Add logic that in case the vmi.Spec.Domain.Devices.Interfaces
have a MAC address for the interface, it would be used to find
the right interface of domain.Status.Interfaces and report back the
name according the vmi.Spec.Domain.Devices.Interfaces name field.
In case there isn't a matching MAC, a name won't be reported.
In case there are two entries with the same MAC on vmi.Spec.Domain.Devices.Interfaces
which match the one reported by the guest agent on domain.Status.Interfaces
the first name will be reported.
Since we don't support bond on SRIOV yet we can't test it, nor it would be a problem yet.
When we do support it, we would be able to
check which other fields can be tested in order to select the right one
(as domain.Status.Interfaces doesnt report type, but, for example,sriov: {} exists on
vmi.Spec.Domain.Devices.Interfaces)

I prefer what I suggested on the comment.
But if there is a good reason for not using then I would try to add the alias and the mac to the hostdev (I guess the mac would be ignored by libvirt, but it will allow us to match between the device and the status).
BTW, do you know why we do the match by the mac address? Isn't the alias reported back by libvirt and can be stored on the status?

According to https://github.com/kubevirt/kubevirt/pull/1468 the alias we are passing on the interface is a user alias. it mean libvirt should override it as will report it back as is. Worth checking if we can use it for the interfaces (and remove agent_parser.MergeAgentStatusesWithDomainData) and for the HostDevice in the case of our PR.

Regarding the approach i this PR, I don't think it is good enough. It solves the bug in a partial way, just if the mac is specified in the spec, which is not always true. Do I miss something?

Tests: Added validation that network name is set in VMI scheme.
Use it also to find the interface name, in order to
not be depended on ethN naming which cant be depend on,
since it might be random each boot.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1856347
Signed-off-by: Or Shoval<EMAIL_ADDRESS>None

But if there is a good reason for not using then I would try to add the alias and the mac to the hostdev (I guess the mac would be ignored by libvirt, but it will allow us to match between the device and the status).

you would still need the mac, so its not a wider solution and needs Aliases and matching,
same lady different clothing, in case i dont miss anything
if we can take it to another PR i would be glad.
mac is good to have in several cases so we would be able to match the right interface and not depend on their boot order
https://bugzilla.redhat.com/show_bug.cgi?id=1874096#c14
PR descriptions says that if mac isnt available the name wont be presented.

But if there is a good reason for not using then I would try to add the alias and the mac to the hostdev (I guess the mac would be ignored by libvirt, but it will allow us to match between the device and the status).

You would still need the mac (in the spec, as this PR need, for adding Alias.Name and Mac in converter.go), so its not a wider solution and needs Aliases and matching logic,
Its the same lady different clothing, in case i dont miss anything, (unless we would do elimination, and know to match the non matched with some other hints, if exists)
Regular interfaces are reported with the mac, under domain.Spec.Devices.Interfaces at the end,

They are reporting the mac because we put it there in podinterface.go.
Same can be done for the sriov nic. The only issue I'm not sure about is whether we have the sriov nic on the pod before attaching it to the vm.

but HostDev arent reported with the mac, so its not symmetric.
    <interface type='ethernet'>
      <mac address='02:00:00:a9:71:f9'/>
      <target dev='tap0' managed='no'/>
      <model type='virtio'/>
      <mtu size='1440'/>
      <alias name='ua-default'/>
      <rom enabled='no'/>
      <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/>
    </interface>

Here when i added the Alias.Name to the HostDev, we dont have mac but.
    <hostdev mode='subsystem' type='pci' managed='yes'>
      <driver name='vfio'/>
      <source>
        <address domain='0x0000' bus='0x65' slot='0x08' function='0x5'/>
      </source>
      <alias name='ua-sriov-net'/>
      <address type='pci' domain='0x0000' bus='0x06' slot='0x01' function='0x0'/>
    </hostdev>

if we can take it to another PR i would be glad.

Why another PR. This PR doesn't fully solves the bug. And if we would be able to use the approach I'm suggesting we will have to change the logic in this PR.

mac is good to have in several cases so we would be able to match the right interface and not depend on their boot order

Can you please explain how it is related to the subject of the comment? I think I miss something.

https://bugzilla.redhat.com/show_bug.cgi?id=1874096#c14
Here we can see an order mix from yesterday, we see the default got the name eth1
    interfaces:
    - interfaceName: eth1
      ipAddress: <IP_ADDRESS>
      ipAddresses:
      - <IP_ADDRESS>
      mac: 02:00:00:48:6b:89
      name: default
    - interfaceName: eth0
      ipAddress: fe80::40dd:13ff:fe9a:e6ec/64
      ipAddresses:
      - fe80::40dd:13ff:fe9a:e6ec/64

(disregard the missing mac, not related to the discussion)
PR descriptions says that if mac isnt available the name wont be presented.

But if there is a good reason for not using then I would try to add the alias and the mac to the hostdev (I guess the mac would be ignored by libvirt, but it will allow us to match between the device and the status).

You would still need the mac (in the spec, as this PR need, for adding Alias.Name and Mac in converter.go), so its not a wider solution and needs Aliases and matching logic,
Its the same lady different clothing, in case i dont miss anything, (unless we would do elimination, and know to match the non matched with some other hints, if exists)
Regular interfaces are reported with the mac, under domain.Spec.Devices.Interfaces at the end,

They are reporting the mac because we put it there in podinterface.go.
Same can be done for the sriov nic. The only issue I'm not sure about is whether we have the sriov nic on the pod before attaching it to the vm.

maybe when we attach we can remember the Bus/Slot/Func if they are the same that later reported by libvirt in order to match ?
<EMAIL_ADDRESS>so far i see that we dont have the mac in PlugPhase1 PlugPhase2 in the iface struct itself if i try to print it,
other interfaces types in this case generate it at their preparePodNetworkInterfaces function.
still resreaching

but HostDev arent reported with the mac, so its not symmetric.
    <interface type='ethernet'>
      <mac address='02:00:00:a9:71:f9'/>
      <target dev='tap0' managed='no'/>
      <model type='virtio'/>
      <mtu size='1440'/>
      <alias name='ua-default'/>
      <rom enabled='no'/>
      <address type='pci' domain='0x0000' bus='0x02' slot='0x00' function='0x0'/>
    </interface>

Here when i added the Alias.Name to the HostDev, we dont have mac but.
    <hostdev mode='subsystem' type='pci' managed='yes'>
      <driver name='vfio'/>
      <source>
        <address domain='0x0000' bus='0x65' slot='0x08' function='0x5'/>
      </source>
      <alias name='ua-sriov-net'/>
      <address type='pci' domain='0x0000' bus='0x06' slot='0x01' function='0x0'/>
    </hostdev>

if we can take it to another PR i would be glad.

Why another PR. This PR doesn't fully solves the bug. And if we would be able to use the approach I'm suggesting we will have to change the logic in this PR.

mac is good to have in several cases so we would be able to match the right interface and not depend on their boot order

Can you please explain how it is related to the subject of the comment? I think I miss something.

https://bugzilla.redhat.com/show_bug.cgi?id=1874096#c14
Here we can see an order mix from yesterday, we see the default got the name eth1
    interfaces:
    - interfaceName: eth1
      ipAddress: <IP_ADDRESS>
      ipAddresses:
      - <IP_ADDRESS>
      mac: 02:00:00:48:6b:89
      name: default
    - interfaceName: eth0
      ipAddress: fe80::40dd:13ff:fe9a:e6ec/64
      ipAddresses:
      - fe80::40dd:13ff:fe9a:e6ec/64

(disregard the missing mac, not related to the discussion)
PR descriptions says that if mac isnt available the name wont be presented.

Please consider that a real use case would have KMP setting the MAC as known,
in case its opt-out it would mean the mac is changed from time to time, and it might not be a real use case.
Anyway i am still investigating, i cant see it on sysfs, nor on ip link, but there might be an option as the sriov plugin / CNI accessing the mac.

/test pull-kubevirt-e2e-kind-1.17-sriov

@AlonaKaplan
@EdDev
As we discussed, please take a look, if its good for phase 1.

/test pull-kubevirt-e2e-kind-1.17-sriov

rebased on
https://github.com/kubevirt/kubevirt/pull/4470
so i can run the lane here
/test pull-kubevirt-e2e-kind-1.17-sriov

and the Alias.Name is missing because a VFIO device type doesn't support getting the mac address at the current stage.

I do not understand this. How is the alias related to the inability to fetch the mac address from the VFIO device?
Please try to phrase this in a way that a reader can understand without assuming he knows what alias or vfio device means here.

Since we don't support bond on SRIOV yet we can't test it, nor it would be a problem yet.
When we do support it, we would be able to
check which other fields can be tested in order to select the right one
(as domain.Status.Interfaces doesn't report type, but, for example,sriov: {} exists on
vmi.Spec.Domain.Devices.Interfaces)

AFAIK bond cannot be create inside the VM as the mac address of the VF cannot be changed. The options to make it trustful exists on some HW devices, but we cannot and should not assume it.
Adding this section is just confusing. Please drop it.
It is enough to mention that duplicate mac/s are treated as first-match as you did.

2nd Phase would either include MAC generating in case the MAC isnt set in the spec,
or using CNI reported MAC, BDF once its supported (it in case it wont involve mandatory
sriov-operator as Petr mentioned).

The limitation has been mentioned already (matching mac), the information provided here does not say a lot to someone who is not in the details (deeply). Either drop it, or phrase it in a way that it will be understood in a general way.
Please also drop any name references of individuals.

Currently CNV supports displaying the SRIOV Network name, only if a MAC exists in the VMI spec,
either set manually or using KubeMacPool.

Per what I managed to understand today  from @RamLavi , KMP is working only on the VM (template) object and not VMI/s, therefore, when looking on the VMI, it only matters if this is set or not.
We should not mention CNV or KMP.
I suggest something like this instead:
VMI status displays SRIOV interfaces with their network name only when they have originally been defined with an explicit MAC address in the VMI spec.

and the Alias.Name is missing because a VFIO device type doesn't support getting the mac address at the current stage.

I do not understand this. How is the alias related to the inability to fetch the mac address from the VFIO device?
Please try to phrase this in a way that a reader can understand without assuming he knows what alias or vfio device means here.

Since we don't support bond on SRIOV yet we can't test it, nor it would be a problem yet.
When we do support it, we would be able to
check which other fields can be tested in order to select the right one
(as domain.Status.Interfaces doesn't report type, but, for example,sriov: {} exists on
vmi.Spec.Domain.Devices.Interfaces)

AFAIK bond cannot be create inside the VM as the mac address of the VF cannot be changed. The options to make it trustful exists on some HW devices, but we cannot and should not assume it.
Adding this section is just confusing. Please drop it.
It is enough to mention that duplicate mac/s are treated as first-match as you did.

2nd Phase would either include MAC generating in case the MAC isnt set in the spec,
or using CNI reported MAC, BDF once its supported (it in case it wont involve mandatory
sriov-operator as Petr mentioned).

The limitation has been mentioned already (matching mac), the information provided here does not say a lot to someone who is not in the details (deeply). Either drop it, or phrase it in a way that it will be understood in a general way.
Please also drop any name references of individuals.

Currently CNV supports displaying the SRIOV Network name, only if a MAC exists in the VMI spec,
either set manually or using KubeMacPool.

Per what I managed to understand today from @RamLavi , KMP is working only on the VM (template) object and not VMI/s, therefore, when looking on the VMI, it only matters if this is set or not.
We should not mention CNV or KMP.

I know that KMP works on the VM, i mentioned VMI because VMI can exists without VM, but i will take all you wrote under consideration, thanks

I suggest something like this instead:
VMI status displays SRIOV interfaces with their network name only when they have originally been defined with an explicit MAC address in the VMI spec.

something is wrong on the test, checking it (worked on the previous version of the test)
had to change because 4001 was mergd

Changes:
Rebased again on 4470.
Added Eventually, because the first report of Mac + Name takes time, due to the logic of guest agent polling by kubevirt which miss the first update and then waits 2 minutes (we will fix it in other PR).
Spitted the functions, in order to run the skip as late as possible, so this PR logic will be tested (unless a skip in the middle is a no go, and then we need to add a test until the mentioned issues will be fixed (Quique works on a PR)).

2nd Phase would either include MAC generating in case the MAC isnt set in the spec,
or using CNI reported MAC, BDF once its supported (it in case it wont involve mandatory
sriov-operator as Petr mentioned).

Please remove the fact that Petr mentioned it from the PR description.

Updated description according comments, please take a look
(if need to align the commit message of the middle commit let me know please)

/assign @EdDev

Rebase to fix SRIOV lane

Added waitForVMIGuestAgentReport, see please
https://github.com/kubevirt/kubevirt/pull/4335#discussion_r515930360

Inc waitForVMIGuestAgentReport time, saw it fails locally once.

It is enough to mention that duplicate mac/s are treated as first-match as you did.

Done, but btw, according this logic no two interfaces would be even displayed, so if there is a case of double
that should be displayed its a bug right?
https://github.com/kubevirt/kubevirt/pull/4335/files#diff-7a3f9e0461f554b07ac195375eb0f8db7d46579071795a65c459c721b1a8204aR555
reason is that its a map that would override the entry in the 2nd time it will see the interface,
and since the HostDev interfaces dont appear in the Spec.Devices.Interfaces, it will set them only according the
domainInterfaceStatusByMac, which doesnt have duplicates

Added waitForVMIGuestAgentReport to 3985, i can confirm it fixed all flakes i saw beside:
https://github.com/kubevirt/kubevirt/pull/4335#issuecomment-720573589
which is much deeper and should be handle on other PR.

Added waitForVMIGuestAgentReport to 3985, i can confirm it fixed all flakes i saw beside:
#4335 (comment)
which is much deeper and should be handle on other PR.

Thanks.
I still suggest extracting  it to a different PR.  Once this fix is in, we might even can consider to return the skipped tests.
and maybe add some more tests (sriov + vlan :) )
(re raising the discussion because the relevant comment was marked as resolved.)

Rebase

Addressed Edy comments,
can you check please https://github.com/kubevirt/kubevirt/pull/4335#issuecomment-720573589
when you review the e2e tests ?
Also please note that this PR fix flake of 3985 (see https://github.com/kubevirt/kubevirt/pull/4335#discussion_r517127980)
Can we do unit test in a follow PR please, immediately after this one ?

I am still missing the unit tests (or a good reasoning not to have them).

There is no unit test at all for SRIOV atm, if we can wrap up this PR and do immediately on a new PR i would be glad,
as they are not specific to this PR because they are missing.

Addressed Edy's Comments (beside unit-tests), see please
https://github.com/kubevirt/kubevirt/pull/4335#issuecomment-723578685

Added Unit test

Changed the Eventually to use 3*time.Minute, time.Second instead of numeric values

Addressed part of the comments, will write once i address all (from now on, in case i do it in few pushes)

Addressed comments - done
see please 2nd paragraph here https://github.com/kubevirt/kubevirt/pull/4335#discussion_r520335688
about what i did with the commits and the code.

Thank you for the co-op
/hold
since we cherry-pick 4511 here
but meanwhile it can be reviewed by maintainer please

1st push - Addressed comments (see inline please for the ones that i think we shouldn't)
2nd push  - Rebase

Missing letter in commits message

Addressed comments according doctor conclusion

/approve

Thanks for the co-op Alona

Updated comment according comment

https://prow.apps.ovirt.org/view/gcs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/4335/pull-kubevirt-e2e-kind-1.17-sriov/1326885829884252160
Passed

Thanks for the review,
please remember that this PR is blocked on #4511 (lgtmed) because it cherry-pick it.

Thanks for the review,
please remember that this PR is blocked on #4511 (lgtmed) because it cherry-pick it.

You have an approve and lgtm, it is up to you if you want to wait or not. You could just rebase the other one after/if this one gets merged first.

Thanks for the review,
please remember that this PR is blocked on #4511 (lgtmed) because it cherry-pick it.

You have an approve and lgtm, it is up to you if you want to wait or not. You could just rebase the other one after/if this one gets merged first.

Hmm the other one is nothing but the first commit here, (if this one gets merged, the other would become empty)
and those are 2 distinct issues,
lets see if the other would be approved in weekend and then rethink please.

Rebased
/hold cancel

Cosmetics according comments

/retest
sriov passed
https://prow.apps.ovirt.org/view/gcs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirt/4335/pull-kubevirt-e2e-kind-1.17-sriov/1328322164876644354


how do i make it work with an interaction, because if i put in the the interaction instead of a message it throwns an error

^ title

Use #playVoiceChannel instead
https://distube.js.org/#/docs/DisTube/stable/class/DisTube?scrollTo=playVoiceChannel

Use #playVoiceChannel instead
https://distube.js.org/#/docs/DisTube/stable/class/DisTube?scrollTo=playVoiceChannel

When I try to enter now, it does not appear to be available. Is there anything we can do instead?

It become #play on ve

It is #play on v4
https://distube.js.org/guide/additional-info/update.html#distube-playvoicechannel


[Searchable Snapshot] Design and implement a Snapshot based Store structure for remote storage

Remote Storage will function as a primary or secondary data store in the future based on the cluster configuration
The goal of this issue is to design and implement a Store structure for remote storage based on the snapshot and repository mechanisms
This will extend the work done in #3114 and add implementations to create the Store instance using Snapshots for Remote Storage

Additional Context:

https://github.com/opensearch-project/opensearch/issues/3113
#3114

Tracked as a part of #3991 / #3961
Implementation - #4892


fix: prettifier

Just checking if this was the prettifier issue

what's the issue with a prettier link?

what's the issue with a prettier link?

There was a space to much in the code. I fixed it and merged the other PR from you :)


PR's status remains Open though the commit is merged when using GitHub App tokens

Subject of the issue
The code is committed to dev automatically after all checks are passed with automerge as intended. But then the PR remains in Open state and the branch is not deleted though delete-branch is set to true.
Also, ensured that we have set Automatically delete head branches.
Below is the code snippet that is used. Please advise if we are missing something or it is how it is. Thanks!
Steps to reproduce
      - name: Create Pull Request
        id: cpr
        uses: peter-evans/create-pull-request@v3
        with:
          author: xxx
          base: dev
          branch: xxx
          commit-message: xxx
          committer: xxx
          delete-branch: true
          team-reviewers: |
            xxx
          title: |
           xxx
          token: ${{ steps.xxx.outputs.token }}

      - name: Enable Pull Request Automerge
        if: steps.cpr.outputs.pull-request-operation == 'created'
        uses: peter-evans/enable-pull-request-automerge@v1
        with:
          token: ${{ steps.xxx.outputs.token }}
          pull-request-number: ${{ steps.cpr.outputs.pull-request-number }}
          merge-method: squash

      - name: Approve Pull Request
        if: steps.cpr.outputs.pull-request-operation == 'created'
        uses: juliangruber/approve-pull-request-action@v1
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}
          number: ${{ steps.cpr.outputs.pull-request-number }}

I've moved this issue to the correct repository and reopened it to track its status.

Received a reply from GitHub Support about this bug.

I was able to reproduce the behaviour and have opened an internal issue with the engineering team to help investigate. I don't have an ETA now but will write back to you as soon as I have any news.

In the meantime, the workaround is to use a PAT instead of a GitHub App token.

I received an update from GitHub Support that this bug has been fixed. So feel free to retry using GitHub App tokens with the action.

Works for me. Awesome work Pete.

I'm having issues with this again I think.
When I use a PAT everything works fine - the auto-merge completes. But when I use an App Token with Contents Read & Write, Pull requests Read & Write, and Metadata Read only the auto-merge hangs with a message

@judahrand Was it working for a good while and then you started having issues just recently? Perhaps it's just a temporary problem with GitHub's service. If it's broken for everyone I expect more users will post here soon.

@peter-evans I've had confirmation from GitHub Support that they were able to reproduce my issue and have opened an internal issue. Looks like there are still issues around GitHub App Tokens and auto-merge! ☹

@judahrand Ah, that's not good. Thank you for letting us know. Please keep us updated on the outcome of the internal issue.


Feature request: flavor = "multi_thread" support for #[sqlx::test]

Is your feature request related to a problem? Please describe.
From documentation:

#[sqlx::test] does not recognize any of the control arguments supported by #[tokio::test] as that would have complicated the implementation. If your use case requires any of those, feel free to open an issue.

Thus this issue.
Describe the solution you'd like
#[sqlx::test(tokio_flavor = "multi_thread", tokio_threads = 1)]

Describe alternatives you've considered

Reimplementing #[sqlx::test] manually in my project without a macro

Tedious

A second macro that would spawn a multi-threaded runtime
Having #[sqlx::test] not spawn a runtime for non-async functions May not be feasible due to how the macro is structured

This would allow me to bring my own #[tokio::test] invocation, since the Tokio macros de-asyncifiy the functions they're used on.

Just spawn a multi-threaded runtime by default

Additional context
n/a

I'd accept a PR to support this:
#[sqlx::test(tokio(...))]

and then we don't have to parse ..., we can just pass it directly to #[tokio::test(...)].
The problem is, that's not currently how it works internally (it expands to more than just #[tokio::test]), so we'd need to move some stuff around.


TypeError: cannot concatenate 'str' and 'SMTPServerDisconnected' objects

I just wanted to try the email notification and got the following errors:
Could not send email, got error Connection unexpectedly closed
python2.7[30270]: Error during summary notification: Connection unexpectedly closed
python2.7[30270]: Could not send email, got error Connection unexpectedly closed
python2.7[30270]: Traceback (most recent call last):
python2.7[30270]:   File "/home/bene/tools/poloniexlendingbot/lendingbot.py", line 90, in
python2.7[30270]:     log.log_error(ex.message)
python2.7[30270]:   File ".../poloniexlendingbot/modules/Logger.py", line 97, in log_error
python2.7[30270]:     log_message = self.timestamp() + ' Error: ' + msg
python2.7[30270]: TypeError: cannot concatenate 'str' and 'SMTPServerDisconnected' objects
THe first two Errors seem to be normal as something went wrong with the email notification, but the last one seems to be a python exception!

I think it's a bug in how the exception message is being handled for printing / notifications.
I'll take a look at it.


python312Packages.yalesmartalarmclient: 0.3.9 -> 0.4.0

Diff: https://github.com/domwillcode/yale-smart-alarm-client/compare/refs/tags/v0.3.9...v0.4.0
Changelog: https://github.com/domwillcode/yale-smart-alarm-client/releases/tag/v0.4.0
Description of changes

Things done

Built on platform(s)

[ ] x86_64-linux
[ ] aarch64-linux
[ ] x86_64-darwin
[ ] aarch64-darwin

For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)

[ ] sandbox = relaxed
[ ] sandbox = true

[ ] Tested, as applicable:

NixOS test(s) (look inside nixos/tests)
and/or package tests
or, for functions and "core" functionality, tests in lib/tests or pkgs/test
made sure NixOS tests are linked to the relevant packages

[ ] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
[ ] Tested basic functionality of all binary files (usually in ./result/bin/)
24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)

[ ] (Package updates) Added a release notes entry if the change is major or breaking
[ ] (Module updates) Added a release notes entry if the change is significant
[ ] (Module addition) Added a release notes entry if adding a new NixOS module

[ ] Fits CONTRIBUTING.md.

Add a :+1: reaction to pull requests you find important.

Result of nixpkgs-review pr 335693 run on x86_64-linux 1

5 packages built:

home-assistant-component-tests.yale_smart_alarm
python311Packages.yalesmartalarmclient
python311Packages.yalesmartalarmclient.dist
python312Packages.yalesmartalarmclient
python312Packages.yalesmartalarmclient.dist


bug:

Is there an existing issue for this?

[X] I have searched the existing issues

Package Version
0.10.15
Current Behavior
Trying to use the wagmi in react with typescript + nextjs, but on yarn build, getting a compile error from wagmi:

Failed to compile.

./node_modules/@wagmi/core/dist/contracts-9eb7706c.d.ts:106:40
Type error: '?' expected.

  104 | }, TArgs = AbiParametersToPrimitiveTypes<TAbiFunction['outputs']>, FailedToParseArgs = ([TArgs] extends [never] ? true : false) | (readonly unknown[] extends TArgs ? true : false)> = true extends FailedToParseArgs ? unknown : TArgs extends readonly [] ? void : TArgs extends readonly [infer Arg] ? Arg : TArgs & {
  105 |     [Output in TAbiFunction['outputs'][number] as Output extends {
> 106 |         name: infer Name extends string;
      |                                        ^
  107 |     } ? Name extends '' ? never : Name : never]: AbiParameterToPrimitiveType<Output>;
  108 | };
  109 | type MAXIMUM_DEPTH = 20;

Any idea how to fix this?
Expected Behavior
No response
Steps To Reproduce
No response
Link to Minimal Reproducible Example (CodeSandbox, StackBlitz, etc.)
No response
Anything else?
No response

Fixed by upgrading our typescript version to 4.7.4


"type" variable changed to "feed_type".

"type" renamed as "feed_type".
Done my best.....

This time I tried my best, but still one test FAILED.
I checked each and every line by myself.
Please, tell me my mistake or how can I know my mistake?

much better. You have a flake8 issue. You should be able to install a flake8 plugin on your editor (Sublime text, VS Code etc al have this). Or just copy the flake8 command from here:
$ flake8 --count hnn_core

Note you'll need to have flake8 installed for that, should be available on pip

One more thing sir, what to do after installing flake8....?

closing as the issue has been fixed in another PR


No such file or directory: 'data/obama/track_params.pt'

Hi, when I run the process_data.py, it shows me No such file or directory: 'data/obama/track_params.pt'.
How to fix it?

I meet the same error，did u have fix it？

I meet the same error，did u have fix it？
No, I haven't solved it yet.

I meet the same error，did u have fix it？
No, I haven't solved it yet.

Oh，me too，did u have fixed it？

I meet the same error，did u have fix it？
No, I haven't solved it yet.

Oh，me too，did u have fixed it？

Try reinstalling pytorch3d, my version is 0.7.4, and the torch version is 1.12.1+cu116.

thanks，I‘ll try it

I have summarized the deployment process of rad nerf, https://blog.csdn.net/BarryA11en/article/details/132894702?spm=1001.2014.3001.5501


Guides for passwordless support

Apologies since this is not exactly an issue, but I was just looking to find out whether there is any estimate on when the additional guides or examples would be created, or if there are any other resources I could be pointed to. I've been trying to get the server portion of the library working to support passwordless but I've been having difficulty figuring it out. Not sure when the notice was added to the docs, so wanted to ask for a clarified update.
Thanks!

Yeah, it's true I've kinda slept on the whole cookbooks thing. Part of the reason is that a Microsoft colleague and I are trying to make https://passkeys.dev the definitive developers guide on how to implement passkeys, at least from a raw JS perspective (we don't want to be prescriptive about use of any front end dependencies like mine.)
That said, this issue you logged got the wheels turning in my head, and I think I might actually make SimpleWebAuthn more opinionated to encourage adoption of passkeys. That is, I'll change the defaults of some values in the four core @simplewebauthn/server methods to tune output for passkeys use cases, but not remove the ability to override the options so that sites using this lib for, say, second-factor-only can continue to do so.
In the mean time, if you can help me understand what difficulties you've had in trying to get SimpleWebAuthn to work when implementing passkeys, please share them here. Your observations will help me with this effort, including when I document all this on https://docs.simplewebauthn.dev.

Oh yeah, there's currently a page for passkeys support here:
https://simplewebauthn.dev/docs/advanced/passkeys
Feedback on docs content is always welcome.

I was a bit confused about generating registration options for a new user. My guess was that a check should be done to determine whether the user already has an account/credentials in the database, and if not, then pass the desired userID and userName into the object passed to generateRegistrationOptions?

You shouldn't go full usernameless, the functionality isn't there yet in the spec to update a credential's username after creation if you allow for anonymous account creation, and then let them choose a username afterwards. That means it's highly recommended that you identify the user before you allow any authenticator to be registered.
How you do that is specific to your project, but that's my advice. Do an email confirmation link or something, then when they click the email to confirm their address use that to bootstrap a registration ceremony. At that point you can know a username (their username), and generate a user ID for them, to use in registration options.

However, I was unable to grasp how I could properly modify it to support a passwordless only option. I also realized I was ignoring storing the current challenge and checking against it later.

@MasterKale I had this exact same problem when started to learn how to use that. I still have some doubts: which is the best way to store the current challenge? Should it be in a specific DB? Memory-based DB?

I had this exact same problem when started to learn how to use that. I still have some doubts: which is the best way to store the current challenge? Should it be in a specific DB? Memory-based DB?

I like using Redis for this because you can setex() the challenge to expire in N minutes, or immediately upon receiving a response and attempt a validation (you only ever want to allow a challenge to be used once, even if the response fails verification.) A session ID in a cookie is an easy way to then pull up the challenge when you get back a response - when you store the challenge in Redis you can use the session ID as its key.
Redis might be considered overkill for this; storing the challenge in a typical DB is also an option. With an RDBMS you'd need to remember to delete the challenge after you get back the response that should have signed it. The technique I described above generally covers how challenges should be handled when working with this stuff.

I was a bit confused about generating registration options for a new user. My guess was that a check should be done to determine whether the user already has an account/credentials in the database, and if not, then pass the desired userID and userName into the object passed to generateRegistrationOptions?

You shouldn't go full usernameless, the functionality isn't there yet in the spec to update a credential's username after creation if you allow for anonymous account creation, and then let them choose a username afterwards. That means it's highly recommended that you identify the user before you allow any authenticator to be registered.
How you do that is specific to your project, but that's my advice. Do an email confirmation link or something, then when they click the email to confirm their address use that to bootstrap a registration ceremony. At that point you can know a username (their email address), and generate a user ID for them, to use in registration options.

Thanks for sharing, that sounds like a great idea! Happy to know about any best practices so that I can follow them. I wasn't planning on going usernameless, but I wasn't sure how or when would be the best place to formally assign a username and ID into the registration process. Your suggestion of bootstrapping after email confirmation seems to be the safest way that makes sense.
Thanks again!

I think this has turned into a good discussion-like topic, so I'm going to convert this over in case we want to continue this conversation.


Add finish-args-unnecessary-xdg-config-access exception for io.github.realmazharhussain.GdmSettings

Read access to $XDG_CONFIG_DIR/monitors.xml is required by GDM Settings for applying current/session display settings to GDM login manager.

Rebuild the app after a while, and it should be fine.

Thanks.


Train model with new datasets

[x] Add new data (@jcohenadad)
[x] Curate data (name file properly according to BIDS, move files to derivatives, etc.) (@kiristern)
[ ] Check segmentations (@mchen1110 )
[ ] Train new model (@uzaymacar & @kiristern & @Nilser3 ) --> let's wait, it is likely that @Nilser3 will be assigned this task. We can discuss that in a meeting, I will clarify.

I copied the new MP2RAGE dataset under duke:mri/basel/INsIDER_SCT_Segmentations_COR. @kiristern as we discussed would you mind BIDSifying these data and putting them somewhere temporary (eg: duke:temp/kiri) and then I will review and put it in our git-annex data server. Few things:

could you confirm you have read access to the data on duke?
there are scripts that Alex used to BIDSify the data (you don't have to start from scratch): here: data.neuro.polymtl.ca:/datasets/basel-mp2rage/scripts
Importantly: some of the patients overlap with what is in data.neuro.polymtl.ca:/datasets/basel-mp2rage so we should make sure to not duplicate subjects. I think these are the first 35 patients (starts with "P", and controls are "C") but this needs to be verified.
I notice that there is only one segmentation file for these updated data, while there are two segmentation files for the 35 patients we already have. I'll ask who did the segmentations so we can mention this in the JSON metadata file
On that note: the current dataset of 35 patients has in fact 3 segmentation files. The 3rd is labeled under my name. @uzaymacar do you remember how it was generated? is this 3rd segmentation the majority voting of the first 2? Or is that a 'curated' segmentation revised by me?

@jcohenadad sorry for the delay. I just checked and I don't have access to duke/mri

@jcohenadad sorry for the delay. I just checked and I don't have access to duke/mri

on it :-)

@jcohenadad data is uploaded to duke/temp/kiri/bidsify.
i wasn't sure what to assign to Author in the json key, so left it blank for now.

wondering about issue 1202? I guess for now since no changes have been established, keeping as is.
(However, I just realized the file names contain caps — I’ll modify that tmrw and re-upload with the corrected file names)

@kiristern Thank you for curating the data. I looked at them and the file name does not correspond to the BIDS convention. For example, here's what you created:
sub-P036_MP2R_INsIDER_SCT_Seg_COR.nii.gz

whereas it should look like this one (from our already existing database):
sub-P001_UNIT1.nii.gz

Also, the JSON file has the extention nii.json instead of json.
Also, I suggest you use an already existing script to BIDSify the data (the same that was used for generating the initial 35 patients data, located under scripts/ in the BIDS dataset). Notably, there is a script called curate_basel_additional_data.py, so you could give it a try, and modify it if needed? And then, pls put it under a script/ folder in the dataset you share under duke so we make sure to include your modified script

ahh sorry, i misunderstood. i filtered out the duplicate patients based on the same sub-PXXX but didn't realize that the file ending needed to match as well.. makes sense. thanks for noticing the .nii.json too. I'll update this evening.
oh i used the curate_basel.py not _additional_.

No worries, let me know when I can review it again 😊

Okay, fixed. but before uploading to server for review, just want to confirm whether "Author" should be the same as the person indicated in the curate_basel_additional_data.py script, or leave blank?

Okay, fixed. but before uploading to server for review, just want to confirm whether "Author" should be the same as the person indicated in the curate_basel_additional_data.py script, or leave blank?

good question. I've just emailed our collaborators to ask. Once we have the answer, please make sure to not edit the py. file, but instead create a new one that will be specific to this dataset. Eg: curate_basel_additional_data_20221028.py and edit the comment at the top of the python file with the relevant info-- thanks

sounds good, thanks. And yes, I actually made a new script and saved in on a new branch in the model_seg_ms_mp2rage repo

could you please open a PR? I already have some comments to give on the script

Follow-up on https://github.com/ivadomed/model_seg_ms_mp2rage/issues/49#issuecomment-1294986243, both the SC segmentations and MS lesion segmentations were performed by "Tsagkas Charidimos".

okay i uploaded the corrected data with Tsagkas Charidimos as the author to duke/temp/kiri/bidsify and i'll create a PR now

Done 😊


Vungle mediation adapter causes videos to randomly pause

MAX SDK Version
7.1.1
Device/Platform Info
iOS iPhone 15 pro / Expo react native app
Current Behavior
Whenever an ad has been showed once, even if its no longer visible, videos showed on any screen will start to randomly pause. Somehow this is connected to this vungle adapter. Removing it from the app removes the problem.
Vidoes are rendered using expo-av.
Expected Behavior
Videos should not randomly pause.
How to Reproduce
Build an app using expo 51 and expo-av
Add the vungle adapter pod AppLovinMediationVungleAdapter.
Reproducible in the demo app?
Yes
Additional Info
The pod is used in a react native app.
No response

@emilnilimaa, could you update our React Native package to version 8.0.0 and try again? We’ve recently updated our native SDKs along with the adapters.
Currently, we don’t support Expo, so you’ll need to run it on your own. Is the issue specific to Vungle? Do other adapters, such as Google AdMob, work with expo-av? If it’s specific to Vungle, there might be a conflict or issue between the native components of expo-av and Vungle.

@emilnilimaa, could you update our React Native package to version 8.0.0 and try again? We’ve recently updated our native SDKs along with the adapters.
Currently, we don’t support Expo, so you’ll need to run it on your own. Is the issue specific to Vungle? Do other adapters, such as Google AdMob, work with expo-av? If it’s specific to Vungle, there might be a conflict or issue between the native components of expo-av and Vungle.

Yeah will try with 8.0.0 soon. Currently this version uses some newer dependencies which our other ad serving module does not have so we need for it to get updated on their end or our builds fail (react-native-google-mobile-ads). We're pretty happy with the applovin integration so far, so might actually just remove that one so we can upgrade.
It seems to be only the vungle adapter that is causing this problem right now. We've tried removing it from our build and the problems disappeared.
We are currently using the following adapter:
pod 'AppLovinMediationByteDanceAdapter'
      pod 'AppLovinMediationFacebookAdapter'
      pod 'AppLovinMediationFyberAdapter'
      pod 'AppLovinMediationUnityAdsAdapter'
      pod 'AppLovinMediationIronSourceAdapter'
      pod 'AppLovinMediationGoogleAdapter'
      pod 'AppLovinMediationInMobiAdapter'
      pod 'AppLovinMediationChartboostAdapter'

None of these seem to cause any video problems with expo-av so far.


Create plex.md and Glances.md and include details

What this PR does / why we need it:
Adds documentation for Plex Media Server

Hey, I messed up creating a pull request! I thought I could just create one pull request, then commit another change to my fork and commit a second pull request. Turns out, it just merges the two in to one pull request!
Either way, this just adds documents for Glances and Plex.

:+1:


Allow increment of hour and minute

This pull request makes it possible that you only select minute quarters.
It allows you to set the the incremental step size for hours and minutes.
So for example if you set hourstep = 2, then you can only select even hours. minutestep = 5 and you can only select minute 5, 10, 15 etc.
Also fixed that AM/PM was not shown on initializing of the hourview (only after toggling am/pm)

This merge does work. I hope @weareoutman merges it soon (I did it manually for my project).

Yea i also added both as my own here https://github.com/JordyMoos/clockpicker so i can continue with it

Nice. Have the changes also landed in the minified version?

Yes they have been updated

AAAHNOOOOIICCEEE

I mailed the maintainer 3 weeks ago with the question if he is still maintaining this but didn't get any response.

:+1:


Fix sign of phase shift in DataSet.apply_symop

Based on visual inspection of maps, it appeared that DataSet.apply_symop() was applying phase shifts in the opposite direction of the specified symmetry operation. This PR changes the sign for how phase shifts are applied, and updates the relevant test.

To do:

It would be useful to make a "gold standard" pair of F/phi with a known real-space translation between the corresponding maps so that we can write a cleaner test.

Codecov Report

Merging #63 (ca755a6) into main (cb5b764) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
- Coverage   98.88%   98.88%   -0.01%
==========================================
  Files          38       38
  Lines        1441     1440       -1
==========================================
- Hits         1425     1424       -1
  Misses         16       16

Flag
Coverage Δ

unittests
98.88% <100.00%> (-0.01%)
:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

reciprocalspaceship/dataset.py
99.02% <100.00%> (-0.01%)
:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dc62ba...ca755a6. Read the comment docs.

As per our conversation offline, let's go ahead and merge this and then do a refactor.

I am still working on this for the complex structure factors. This test may not be working as well as I had hoped, but I am not yet ready to come to that conclusion

This PR now fixes DataSet.apply_symop() for use with both |F|+phi and complex SFs. I updated the test to use a P1 dataset avoid an interdependency caused by the use of apply_symop() in DataSet.to_reciprocalgrid().
P.S. -- I updated my emacs recently to auto-apply black formatting, which is why there are additional changes to the test_dataset_symops.py file


fvwm related service files. [needs attention]

Hello;
trying to add
    xserver.desktopManager.default = "none";
    xserver.desktopManager.fvwm.enable = false;
    xserver.windowManager.default = "fvwm";
    xserver.windowManager.fvwm.enable = true;

causes to return errors about the service doesn't exist. So please ad them.
this option is needed to set fvwm for display-managers like lightdm or sddm.
Thanks

Linux noisecrew 4.9.17 #1-NixOS SMP Wed Mar 22 11:44:07 UTC 2017 x86_64 GNU/Linux

nix-env (Nix) 1.11.8

FYI, you can do this yourself (as I have been):
    services.xserver.desktopManager.default = "fvwm";
    services.xserver.desktopManager.session =
      [ { manage = "desktop";
          name = "fvwm";
          start = ''
            ${fvwm}/bin/fvwm &
            waitPID=$!
          '';
        }
      ];

However, now that I'm not just getting started with Nix, I should really commit that...  I'll add it as my next nixpkgs TODO.

rror: undefined variable ‘fvwm’ at /etc/nixos/services.nix:20:15
(use ‘--show-trace’ to show detailed location information)
building Nix...
error: undefined variable ‘fvwm’ at /etc/nixos/services.nix:20:15
(use ‘--show-trace’ to show detailed location information)
building the system configuration...
error: undefined variable ‘fvwm’ at /etc/nixos/services.nix:20:15
(use ‘--show-trace’ to show detailed location information)

Ah; I had a bit of other infrastructure that I filtered out.  Try using ${pkgs.fvwm} instead?

wouldn't be better to add it to nixos tree.

cc @ttuegel

Given that the module has now been added and is available in nixos-unstable, I think this issue can be closed?


Add android client to the repo

Add android client to the repo

@pratikvelani Looks like there are some linter issues, one with mixed line endings and other about whitespace. I think the whitespace one we can ignore for that directory if Android Studio is consistently violating the linter here.

@pratikvelani ~Could you set up commit signing for Git? I'll override for now but our branch protection rules require them.~

EDIT: Nevermind, I see you've set it up!

Yupe, I did the first commit from my MacBook it didnt have the gpg keys setup.


add step chain parentage map to reqmgr db.

related to #8697
Alan as we discussed I added a couple of method on workload helper.

Could you check the format, name and structue? If it is acceptable. I will add the unittest.

Added unittest and also tested in vm. Seems to working OK.
https://reqmgr2-dev.cern.ch/reqmgr2/fetch?rid=sryu_StepChain_DupOutMod_syu_180725_221522_2125
"StepChainParentageMap": {
    "Step3": {
      "ParentDsets": "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM-RAW",
      "ChildDsets": [
        "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/AODSIM",
        "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM-RECO"
      ]
    },
    "Step2": {
      "ParentDsets": "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM",
      "ChildDsets": [
        "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM-RAW"
      ]
    },
    "Step1": {
      "ParentDsets": null,
      "ChildDsets": [
        "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM"
      ]
    }
  },

Alan, I updated the places you pointed. Thank you for the review.
Tested in vm again. It seems working ok.
"StepParentageMap": {
    "Step3": {
      "ParentDset": "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM-RAW",
      "ChildDsets": [
        "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/AODSIM",
        "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM-RECO"
      ]
    },
    "Step2": {
      "ParentDset": "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM",
      "ChildDsets": [
        "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM-RAW"
      ]
    },
    "Step1": {
      "ParentDset": null,
      "ChildDsets": [
        "/RelValProdMinBias/Integ_Test-StepChain_DupOutMod_StepChain-v11/GEN-SIM"
      ]
    }
  },

test this please

@amaltaro, Alan, should I merge this one? I can fix some of the pylint errors. But since you are working on https://github.com/dmwm/WMCore/pull/8706. I will have the bigger conflict. Just let me know.


Invalid TzKT API responses are getting cached on the disk

Steps to reproduce: Run dipdup init with incorrect TzKT URL
What did you expect to happen: Fixing URL helps
What actually happened: Invalid response was cached. init command fails until cache clear command is invoked.
Environment

Python version: any
DipDup version or commit hash: any
Database engine version: any
Hasura version: any
Are you in docker? any
Can you reproduce this issue on master? any

#255


Missing holidays in Spain

Hi,
Holiday Easter Monday (10th of April) is missing in the CT subdivision in Spain.
Thanks,

Seems to be fixed in #902. Please, check with beta version, if you can.

Looks good in v0.20
When will be released?

Looks good in v0.20 When will be released?

It's my plan to release it within the next 2 weeks (pessimistic ETA -- by end of this month)

ok, thanks a lot!

Hi, is this fix included in 0.21.13? Today is being shown as a working day to me.

Hi, @cdeharo! Yes, it was included in 0.20. I've just checked:
>>> import holidays
>>> ct = holidays.ES(subdiv="CT")
>>> ct["2023-04-10"]
'Lunes de Pascua'
>>> holidays.__version__
'0.20'

Same with 0.21.13.

Hi, is this fix included in 0.21.13? Today is being shown as a working day to me.

HI @cdeharo,
the issue was resolved in v0.20 and closed by the OP.
Please open a new ticket if you're still experiencing holidays inconsistency.


Use Stitches or Vanilla Extract instead of styled-components?

For reference, stitches and vanilla extract both market themselves as zero-runtime (or near zero-runtime) css-in-js solutions
of the two, stitches appears to have an API that is most similar (almost identical) to styled-components, and appears to include support for variants and theming in a very similar manner/style to styled-system.  this would therefore probably be easier for us to adopt.  it's FAQ notes some limitations in browser support that we should consider.  the library itself is written in JS, but has typescript type definitions.  stitches claims "near zero-runtime" - in reality at runtime it uses two modules which they claim that weigh in at 4.7kb gzipped (altho according to bundlephobia those two modules are 5.1kb each, gzipped)
vanilla extract is in some ways slightly more advanced, but in other ways significantly more primitive.  it's written completely in typescript, and appears to be genuinely zero-runtime, with all styles built at compile-time.  it also offers theming support, although this appears to be not quite as well thought out as either styled-system or stitches.  there is some support for variants, but this isn't built into the base style generation functions, and so has some overhead to use this feature.  the API differs significantly from styled-components, essentially because it is only a CSS generator and has no integration with React at all.  thus it would be significantly harder to adopt.  given the number of components we have, the amount of glue-logic we'd need to add to our components to make use of these styles could end up making our overall bundle size larger.
I've not looked into it, but another alternative could be linaria - that too claims to be "zero-runtime", and offers React integration with an API very similar to styled-components
on balance tho, having taken a look, I'm not sure what significant advantage we would gain from moving away from styled-components to one of these choices.  the runtime memory footprint may be slightly lower, but

I'm not sure what significant advantage we would gain from moving away from styled-components to one of these choices

I would like govuk-react to be a realistic choice for doing server-side only rendered applications with the option of turning on client side rendering as an enhancement, and would like it to be able to pass the service standard so it can be used for services and not just internal applications. I think rendering CSS files would be a good step in that direction
I'm not sure about the IE11 compatibility requirement for services, but perhaps we could merge in the change when IE11 is EOL on 17 August, 2021 :D

but would the cost to migrate be worth it?

Once we have visual regression turned back on, we can see if it is as simple as the emotion => styled-components was.
If we use stitches, it looks like it could be fairly simple upgrade.
Lastly, if we use Stitches, we could plug in their primitives API to extend our component offering:
https://github.com/radix-ui/primitives

I've separated Radix discussion/implementation to this ticket:
https://github.com/govuk-react/govuk-react/issues/885

vanilla extract will help with static extraction of styles to CSS files but would be limited when it comes to critical path extraction
Linaria does seem to have both static extraction and critical path but would need to be assessed https://github.com/callstack/linaria/blob/master/docs/CRITICAL_CSS.md
Stitches will help with critical path CSS and is very performant in doing so, but does not currently extract to CSS files. This may be added in future.
If each page had a unique critical path CSS, then extracting this to a CSS file would mean a unique CSS file per page. If the HTML response is cached then it is likely better to include this critical CSS inside the HTML response rather than extracting to a CSS file to avoid a round trip to fetch the CSS.

My initial instinct is that decisions regarding trade offs between critical path CSS and static extraction should be left to the application developer rather than govuk-react, so having something that is flexible for use with both approaches would be preferred.
Next steps:

assess if we will be using govuk-frontend scss files (a govuk-react layer will still be needed for extended styles)
if we use govuk-frontend scss, assess how critical path CSS and static extraction would work with regard to the 'unique critical path css per page' example above, and whether this decision can be left up to the application developer for the scss files.
assess requirements from the govuk-react CSS layer with respect to critical path and static extraction
if we want to leave the choice of critical path CSS vs. static extraction, to the application developer what are our options

I think if/when we move away from styled-components we should choose a solution that is in line with the React team's thinking.
For example, https://github.com/reactwg/react-18/discussions/110#discussion-3619937 contains a few important points:

collect all the rules that were generated during the render into one or more


[ar] Translate setup/_index.md

This PR will create a new file contents/docs/ar/setup/_index.md that is translated into arabic following the english docs.
Following the plan in this PR: https://github.com/kubernetes/website/issues/37546 more file will be picked. The base branch is currently main until a branch is created for arabic.

/label language/ar

/language ar

/language en

Thank you @essamgouda97 for this effort! I cannot seem to find a link that will show me a preview of this page. Have you been able to find it? https://deploy-preview-44289--kubernetes-io-main-staging.netlify.app/ takes us to the homepage, but I'm not able find this page manually by manipulating the URL.

Thank you @essamgouda97 for this effort! I cannot seem to find a link that will show me a preview of this page. Have you been able to find it? https://deploy-preview-44289--kubernetes-io-main-staging.netlify.app/ takes us to the homepage, but I'm not able find the page of this PR manually by manipulating the URL.

@adowair any idea what's missing ? How does that work anyways 😆 ?

you can squash commits and move from draft to ready for review

@mboukhalfa @adowair I want one more shot at improving the language used this weekend then I will squad and mark as Ready for review.
After that I will move on to another page.

@mboukhalfa @adowair this being my first OSS contribution ever I am bit noob-ish, I rebased off dev-1.29-ar.1 but there are files that I didn't edit that are there, is the branch not up to date with main ? My PR is ready for review I will mark it as such once I got it right.

@mboukhalfa @adowair this being my first OSS contribution ever I am bit noob-ish, I rebased off dev-1.29-ar.1 but there are files that I didn't edit that are there, is the branch not up to date with main ? My PR is ready for review I will mark it as such once I got it right.

I cannot see what is really causing this probably you are not forking the correct branch ?  fork dev-1.29-ar.1 create a branch from from it then add you changes better in a single commit if not necessary separate you commits then create a PR from that branch to dev-1.29-ar.1 upstream

This PR is superseded by #45094


Add eval on demand functionality to binary operators

Closes #79

Very cool, thanks a lot for this!


Capture and report failure to launch Docker image

When a docker image fails to run on a node, "Unknown system error" is reported. Please make this a SYSTEM error with text such as "Failed to run docker image".

This can be determined by receiving a task failure message before a task running message.
We should create three new errors:

SYSTEM Error: Command line Mesos task failed to launch
SYSTEM Error: Docker Mesos task failed to launch
ALGORITHM Error: General algorithm failure where no exit code mapping was provided

In description for error 3, include a high level explanation of defining exit code mappings for the algorithm to get better error reporting.
Add method to the base task for being a 'catch-all' in case the specific task logic (pre-task, job-task, post-task) does not come up with an error. Each specific task should call this base logic if they can't come up with an error (instead of defaulting to unknown). The base task method should check if a failure message (other than lost) occurred before a task running message and if so, return new error 1 (is task uses_docker is False) or new error 2 (if task uses_docker is True).
The job task should use new error 3 if a valid failed exit code is provided, but there is no exit mapping for it.

Merged into master


feat(ioctls): Move SGX ioctls to sgx crate

Is there an existing issue for this?

[X] I have searched the existing issues

Description
SGX_IOC_ENCLAVE_RESTRICT_PERMISSIONS has to be called from the enclave, which beans that SGX backend is not an appropriate place to hold the ioctls.
Acceptance Criteria
No response
Suggestions for a technical implementation
No response

This is not necessary but I have clean commit for this so I anyway make the PR available. Logical place for ioctls would be to be here.

IMHO, this would make still sense in order to centralize all of this to single crate, and there is good pragmatic reason to use actual values for ioctl's: you get away without having to add an dependency. They never change (when they're in mainline) and they have named constants so it does not make anyones life more difficult.


Videogular 2 Display Issues on iPad?

Is there a working example somewhere for Videogular 2 functioning on iPad? I'm getting issues both when viewing in xCode iOS Simulator and with an actual iPad, while the example at http://www.videogular.com/alpha/ works on iPhone SE.

Angular2 seems to currently have issues w/the Intl time API. They are documented in the Angular2 repo. e.g. At http://www.videogular.com/alpha/ I get the following error in iOS Simulator: Can't find variable: Intl in [{{ media?.time?.current | date:'mm:ss' }} in SingleMediaPlayer@16:25] Of course, this isn't a problem I'm expecting you to solve, but just a heads up.

In my local environment, if I remove references to time in order to avoid the error above, I get no errors showing up in console but I get an unstyled player with the buttons stacked vertically to the left of the screen, even with the style of height: 50px on the wrapper player per the example.

Please, move this issue to videogular2 repo.
https://github.com/videogular/videogular2/projects


API server API consistency[Feature]

Search before asking

[X] I had searched in the issues and found no similar feature requirement.

Description
Currently, API server APIs are inconsistent:
Cluster and Serve creation requires specifying a namespace in the creation request, while Job does not. The namespace is specified in the request body in all 3 cases, so such inconsistency can be confusing to the users.
Use case
Align all 3 APIs not to require namespace in the URL
Related issues
No response
Are you willing to submit a PR?

[X] Yes I am willing to submit a PR!

/assign

@kevin85421 we can close this, it's fixed by https://github.com/ray-project/kuberay/pull/1447


Allow setting of port for allure report open

Hi,
We have recently moved to Docker and it would be great if the port generated for 'allure report open' could be set with a flag, or at least be deterministic such that we can forward the port to our host on startup of the container.
Best,
Justin

Just looked at the source and I see this is supported with -p , it was not in the wiki. Thanks!


Incorrect message if steer values add up to 0

Describe the bug

When the steer block is supplied with values that add up to 0 the message "The execution of this block will have no effect" is displayed. However, the robot will turn on the spot as expected.
There seems to be some calculation checking for 0 values in the implementation of the steer (or drive blocks in general) that is erroneous.

To Reproduce
Steps to reproduce the behavior:

Add steer block
Add negative and positive values that add up to 0
Open simulation
See wrongly displayed message

Expected behavior

No message.

Screenshots

Resolved by GCI student.


Add global timeout mechanic for PCIC methods

maybe also for RPC?

Maybe we should add a wait parameter to each function which deactivates the timeout mechanic. There are field applications which do not require the timeout.
If the wait parameter is False, a default timeout value should be used.
The customer should also be able to setup an individual timeout, if timeout_secs != -1.
E.g.:

    @timeout(max_timeout=PCIC_COMMAND_TIMEOUT)
    def activate_application(self, application_number: [str, int], wait: bool = True,
                             timeout_secs: float = -1) -> str:
        """
        Activates the selected application.

        Parameters
        ----------
        application_number :
            2 digits for the application number.
        wait :
            If True, the function waits until execution of the command is finished.
            If False and timeout was reached, the function throws an IfmTimeoutError. Defaults to True.
        timeout_secs:
            The duration in seconds of how long to wait for the command to finish. Default is 30 seconds.

        Returns
        -------
        result :
            sensor feedback code

In addition, a retry feature could also make sense at this point.


Advanced Lookup Doc

Can you please also add details if the new advanced lookup support JS code added to the lookup field? Such as added custom view, filtering, etc.

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: c392ce52-689a-f50c-f985-6f8ae7d1a128
Version Independent ID: c3c7e767-f99c-b5e4-8bb6-1875004726c3
Content: Use the lookup column on a row - Power Apps
Content Source: powerapps-docs/user/lookup-field.md
Service: powerapps
GitHub Login: @mduelae
Microsoft Alias: mkaur

@jaimieji I pinged our internal team to provide more info to see if we can add this info. Soon as I hear back, I'll update this thread and the topic if needed.
Thanks,
Mints

@jaimieji I'm updating the topic and adding with this note.
Advanced lookup supports customizations on lookup control through Client APIs like addCustomView, addCustomFilter, addPreSearch and removePreSearch.
Ping me back if you have further questions. Closing the request for now.
Thanks,
Mints


Tapped handler are invoked twice

I'm submitting a...

Bug report

Current behavior
When registering a Tapped event handler on a Button in XAML, the handler is invoked twice when we click on the button.
Expected behavior
It's  being invoked only once.
Minimal reproduction of the problem with instructions
<Button Content="Animate" Tapped="StartAnimation" />

private void StartAnimation(object sender, TappedRoutedEventArgs e)
{
     Console.WriteLine("Tapped");
}

cf. DoubleAnimation_BeginTime in Uno samples.
Environment
Nuget Package: Uno.UI
Package Version(s): _latest dev_
Affected platform(s):
- [?] iOS
- [?] Android
- [x] WebAssembly
- [ ] Windows
- [ ] Build tasks

Visual Studio: _irrelevant_
Relevant plugins: _none_

Fix has been merged to master....


[DOCS] Create a location for links to videos from Community talks and other resources

Problem/Concern
In order to close #780 we need a process for publishing links from recorded presentations and talks, and video resources that can help us build and educate the Theme Builder community.

See discussion notes in #780

Proposed Solution
From discussion with the project team we decided to create a home in the Docs area of this repo and post the link from the State of Open Con 24 talk as a start, and include information in the README or other area(s) about where to find videos from talks and demo videos.

@PaulaPaul this is already done, right? Let us know.

Just needs to be merged: https://github.com/finos/a11y-theme-builder/pull/799

Merged


Interactive figures don't work on latest Anaconda install

FYI, I've been updating my fork of these sklearn materials and the interactive figures aren't working on an up-to-date Anaconda install.
IPython complains that IPython.html.widgets has been deprecated. Used here, here, and here. Also, the previous figure doesn't disappear when a new parameter setting is chosen on the widget.
Also, the interactive tree code is complaining about Dot objects not being subscriptable again.
Currently digging into how to fix these...

thanks... I kinda hope we can get the tree plotting to be a bit more painless by the time scipy comes around...

I kind of hacked it to get it working on a short time constraint. You can find those commits here.
Fixing the imports was easy---they only moved the widgets over to pywidgets.
Fixing the tree plotting was straightforward. I just had to remove the subscript operation.
Fixing the interactive figures was the toughest part. I had to change the %matplotlib inline magics to %matplotlib notebook, otherwise the previously-generated plots weren't going away when I chose a new parameter setting. Not sure on a better solution at the moment.

PR please? ;)

Done. Note that I consider some of these fixes to be temporary workarounds rather than long-term solutions.
I think it's also worthwhile to through the notebooks and make sure that the color schemes still look OK. The latest version of matplotlib changed the default color schemes, and now some figures don't look as intended in the notebooks. I fixed some of the most egregious cases, but there are other cases that still need work.

yeah the matplotlib changes ... I have to fix those for my book first ^^

Close this? Even though this is not the long-term solution, I think the long-term solution will emerge with rewrites...


proposal: encoding/base64: missing function in encode/decode pair

The DecodeString() method exists as a single function, an EncodeString() method should be added as it will not break forward compatibility and conform to a standard naming structure and module offering.
https://pkg.go.dev/encoding/base64
This is not a duplicate of a bug, I was asked to make it a proposal regardless of it being a bug or proposal.
This is a proposal in writing to a software repository.

DecodeString is (b64 encoded string) -> (unencoded []byte).
The reverse is EncodeToString, which does (unencoded []byte) -> (b64 encoded string).
I am not sure what else you are looking for.

Is the function EncodeToString the behavior you want, and you think it should be named EncodeString?

What is the exact API that you are proposing?
We currently have
// DecodeString returns the bytes represented by the base64 string s.
func (enc *Encoding) DecodeString(s string) ([]byte, error)

// EncodeToString returns the base64 encoding of src.
func (enc *Encoding) EncodeToString(src []byte) string

What are you suggesting that we add?  Thanks.

@ianlancetaylor
Do you not understand that the input arguments to those functions are different?
If there is an Encode function there should be a Decode function with the same name structure and:

the same input argument types
the same output argument types

I realize what happened, someone was building it and needing to encode a byte array to a string so they wrote that function, then needed to decode a string to a byte array so they wrote that function.
When people read the documentation first, they are confused by the lack of a common naming structure and input/output type structure.
That should be cleaned and reorganized so that when people are working and reading the 1000's of Go functions (certainly impossible to remember all) they will not get frustrated by constant flipping between string and []byte and a lack of a naming structure that can be quickly remembered because it adheres to both common function names per functionality and input/output type.

You must understand, your company can afford to pay people to do things like that and have a team leader structure that respects who actually made things.
That's why you have things like Gmail and Go, because you let people work and then lead other people to finish their work, not tried to confuse them with constant changes that no single person understands.
It was the same way at Ford, it was the same way at Chevy, it is the same way at Boeing, Lockheed Martin, etc....

@andrewhodel Help us out a bit if you want us to consider your change. Please tell us exactly what method you want added. You've more than adequately defined the motivation - but we're still not clear on what exactly the changes you're proposing are. Write us a prototype of the function/method you want, and a 1-2 sentence godoc description of what it does. Thanks.

@randall77
EncodeString() and DecodeString() should both exist because one of them does and the concept of encoding/decoding is fundamentally bound together.
I didn't read every function, but you should and you should adhere this naming structure and ensure that all relevant sets exist.
In other words
Encode([]byte) means there should be Decode([]byte)
function_1_group_with_3_total() should include function_2_group_with_3_total() and function_3_group_with_3_total() and so on.  For example, with hash functions there is usually a standard naming format due to the required functions, that of input, parse, decode, inputMore etc.
Then when someone is working for example with the encoding/base64 library, they can set a number of days or hours aside to work with the library then get used to the functions.  In other words, they can type Encode and Decode and EncodeString and DecodeString enough times to not need to deal with remembering arguments to every single function instead only needing to remember the top level function names as they all follow a common naming pattern.

Ok, but you still haven't answered my question - what exactly are you proposing to add to encoding/base64?

@randall77
I answered your question in the first sentence of the original reply to your original comment/question.
EncodeString() and DecodeString() should both exist because one of them does and the concept of encoding/decoding is fundamentally bound together.
These are functions in Go of the encoding/base64 library.  One exists, one doesn't.  Again, these are functions in Go.

@andrewhodel That is not an answer to the question to @randall77 and it is not an answer to the question I asked.
I will repeat:
We currently have
// DecodeString returns the bytes represented by the base64 string s.
func (enc *Encoding) DecodeString(s string) ([]byte, error)

// EncodeToString returns the base64 encoding of src.
func (enc *Encoding) EncodeToString(src []byte) string

What are you suggesting that we add? Please write it in the exact form that I wrote it.  This is a programming language.
Precision matters.  Please write down exactly what you mean.  Don't describe what you mean in words.  Write it in Go.  Thanks.

func (enc *Encoding) EncodeString(s string) ([]byte, error)

You should also read and make sure all the other pairs exist for encoding and decoding of, with respect to both input and output:

byte arrays
strings

If you want me to write it, then provide commit access.

You can write any change you like without commit access, as described at https://go.dev/doc/contribute.  Thanks.

@ianlancetaylor then make:
@ianlancetaylor
@prattmic
@findleyr
@cherrymui
All work that way for every commit or stop with the absurdity.

We do all work that way for every commit.  Why do you think we don't?

@ianlancetaylor
That's not true, you push changes directly to the repository.

@andrewhodel That is not correct.  Why do you think it is?
All changes to the Go repository must be reviewed and approved by somebody else, as described at https://go.dev/doc/contribute#review.  That is true for everybody.
What is different for people with approval access is that they only require one reviewer, not two reviewers.  See https://go.dev/wiki/GerritAccess.

You are adjusting time and commits to dev while calling it changes.
Thank You,
Andrew Hodel

On Feb 22, 2022, at 10:58 AM, Ian Lance Taylor @.***> wrote:

@andrewhodel That is not correct. Why do you think it is?
All changes to the Go repository must be reviewed and approved by somebody else, as described at https://go.dev/doc/contribute#review. That is true for everybody.
What is different for people with approval access is that they only require one reviewer, not two reviewers. See https://go.dev/wiki/GerritAccess.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.

You are adjusting time and commits to dev while calling it changes.

I'm sorry, I don't understand what you mean.

I map all that stuff out with https://github.com/andrewhodel/rrd based on word counts and frequency to prove it to people.
Thank You,
Andrew Hodel

On Feb 22, 2022, at 11:06 AM, Andrew Hodel @.***> wrote:
You are adjusting time and commits to dev while calling it changes.
Thank You,
Andrew Hodel

On Feb 22, 2022, at 10:58 AM, Ian Lance Taylor @.***> wrote:

@andrewhodel That is not correct. Why do you think it is?
All changes to the Go repository must be reviewed and approved by somebody else, as described at https://go.dev/doc/contribute#review. That is true for everybody.
What is different for people with approval access is that they only require one reviewer, not two reviewers. See https://go.dev/wiki/GerritAccess.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.

I don't see why EncodeString should return an error. The other Encode functions do not. Base64 encoding and decoding are not symmetric in that encoding should always succeed, whereas decoding might not.
Other than that, seems like a reasonable function to have.  Do you have a situation where you have a string source and wish to encode it to a []byte output? Just wondering if this is a function you actually ran into a need for, or if you just were confused about the lack of a symmetric API.

In any case, thanks for defining precisely what you suggest that we add (although you omitted the doc comment).  I gather you are thinking of something along the lines of
// EncodeString returns the base64 encoding of s.
func (enc *Encoding) EncodeString(s string) ([]byte, error) {
    buf := make([]byte, enc.EncodedLen(len(s)))
    enc.Encode(buf, []byte(src))
    return buf, nil
}

I think the argument that you are making is that this is parallel to the existing DecodeString method.
How often does a need for this method come up in practice?  Do you have any examples of code that would use it?  Thanks.

Start counting them you will find out. It is basic manipulation of words causing the new words to exist.
If you count the number of times someone commits vs the changes approved in that list you will get it.
Thank You,
Andrew Hodel

On Feb 22, 2022, at 11:07 AM, Ian Lance Taylor @.***> wrote:

You are adjusting time and commits to dev while calling it changes.
I'm sorry, I don't understand what you mean.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.

Lack of symmetry.
Yes you need an error, what if the memory bound is reached while reading?
Thank You,
Andrew Hodel

On Feb 22, 2022, at 11:09 AM, Keith Randall @.***> wrote:

I don't see why EncodeString should return an error. The other Encode functions do not. Base64 encoding and decoding are not symmetric in that encoding should always succeed, whereas decoding might not.
Other than that, seems like a reasonable function to have. Do you have a situation where you have a string source and wish to encode it to a []byte output? Just wondering if this is a function you actually ran into a need for, or if you just were confused about the lack of a symmetric API.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.

I map all that stuff out with https://github.com/andrewhodel/rrd based on word counts and frequency to prove it to people.

OK, great.  Do you want to try to prove it to me?
  I don't understand what you could do to demonstrate that I am not telling the truth.  But I am mildly curious as to where the misunderstanding arises.

Yes you need an error, what if the memory bound is reached while reading?

If a Go program runs out of memory, it will crash.  There is no way for a function to return an error because it has run out of memory.

If you count the number of times someone commits vs the changes approved in that list you will get it.

Again, I'm sorry, but I don't understand what you mean.  If you have actual data, can you simply show it?

Again. This is why other libraries have per iteration input functions, for reading streams etc.
That isn’t to say that you could read the input string length, then read the available memory and know what difference the encoding process will require (bytes per chunk for input and output + anything else) and return an error when that is greater than n*available.
That is how things work.
Thank You,
Andrew Hodel

On Feb 22, 2022, at 11:13 AM, Ian Lance Taylor @.***> wrote:

If you count the number of times someone commits vs the changes approved in that list you will get it.
Again, I'm sorry, but I don't understand what you mean. If you have actual data, can you simply show it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.

It is because you aren’t respecting the development branch as a place to read changes before versioned approval.
In other words, why do you have release candidates?
Thank You,
Andrew Hodel

On Feb 22, 2022, at 11:12 AM, Ian Lance Taylor @.***> wrote:

I map all that stuff out with https://github.com/andrewhodel/rrd based on word counts and frequency to prove it to people.
OK, great. Do you want to try to prove it to me?
 I don't understand what you could do to demonstrate that I am not telling the truth. But I am mildly curious as to where the misunderstanding arises.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.

These truths of memory testing (especially when it is very low resource usage to do so) are similar to network programming in a day and age that testing is sold for entertainment as blocking certain tcp packets against protocol.
In other words it has to be dealt with anytime network traffic is greater than the packet MTU so may as well exist at the memory level.
Thank You,
Andrew Hodel

On Feb 22, 2022, at 11:23 AM, Andrew Hodel @.***> wrote:
Again. This is why other libraries have per iteration input functions, for reading streams etc.
That isn’t to say that you could read the input string length, then read the available memory and know what difference the encoding process will require (bytes per chunk for input and output + anything else) and return an error when that is greater than n*available.
That is how things work.
Thank You,
Andrew Hodel

On Feb 22, 2022, at 11:13 AM, Ian Lance Taylor @.***> wrote:

If you count the number of times someone commits vs the changes approved in that list you will get it.
Again, I'm sorry, but I don't understand what you mean. If you have actual data, can you simply show it?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.

The current helper methods assume that base64 is applied to binary data, which is almost always a []byte.
And it is applied to create text data, which is often a string.
So the API uses []byte for the binary (unencoded) data, and string for the text (encoded) data.
This has worked very well in practice.
There is no obvious reason we should add the other pairs, which would at the very least confuse me when reading the API.

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

I guess every middleman your Go servers work with support perfect utf8.
I need to send data to things so old that anything beyond 127 bits is out of the question so they can pass it on and then without base64 what are you going to do?
I know, another go type modification is just what the doctor called for!

@rsc it's simple, you have a string and a validation function that ensures that string is "at protocol" meaning hasn't the distant device doesn't even fully support ASCII, then you need to pass it through that distant device so you base64 encode the validated string and send it.
What you are saying isn't real, there's no where on earth where an operating system requires code to not using bits and those work with Go servers that use strings for simplicity.

@andrewhodel I think it would help a lot if you made an example on https://go.dev/play/ to show your use-case and why this isn't trivially covered by the existing API.

// working with string
var s = "asdfStringWithoutUnicodeMoreThanOneTwentySeven";
function_without_string_support([]byte(s));

Code reviewer: "That is messy code, there are too many type modifications".
I guess they will never figure it out, oh well poof.

No change in consensus, so declined.
— rsc for the proposal review group


feat(firestore-bigquery-export): add support for transforming data via a callable function

Reattempt of #756 + #832 after looking into why NPM publish didn't work last time.

Codecov Report

Merging #849 (3463776) into next (3b1cf99) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             next     #849   +/-   ##
=======================================
  Coverage   50.19%   50.19%
=======================================
  Files          30       30
  Lines        1329     1329
  Branches      197      197
=======================================
  Hits          667      667
  Misses        661      661
  Partials        1        1

Impacted Files
Coverage Δ

firestore-bigquery-export/functions/src/config.ts
88.89% <ø> (ø)

firestore-bigquery-export/functions/src/index.ts
100.00% <ø> (ø)

@apascal07 My only concern here is that the change-tracker will need to be released and published as part of the release process, otherwise the updated package json reference:
"@firebaseextensions/firestore-bigquery-change-tracker": "^1.1.14"
will be unavailable when upgrading the extension.

Publishing dependencies to the NPM registry is part of our release process so this shouldn't be a concern.


fix docs for regex replace operator (.&)

It seems this commit changed the order of arguments without updating the docs.
https://github.com/aya-lang/aya/commit/ff1a162094ba3fa784e7292fce3f13ee15250af2
Usages like https://github.com/aya-lang/aya/blob/53542a60a22de71af90e81ca66eed560722d7ea1/base/importlib.aya#L74
were updated though, so I'm assuming it's just the docs that are wrong.

Thanks!


Url argument in ws and http client.

Personally, I don't like just passing in the url like py client = HTTPClient("http://localhost:8000", namespace="test", database="test", username="root",                              password="root")  because it is a bit confusing and inefficient. For example, if the url localhost is passed the requests made are actually significantly slower, speaking from past experience.
This is due to localhost being an extra local dns conf lookup.
To prevent this, perhaps there could be a config class for url and port. This way both https and http can be tried to ensure the correct protocol is used. This helps because if a user doesn't understand the difference in the protocols they won't receive raw errors.
In addition, reserved ports can be checked, localhost can be changed to <IP_ADDRESS> and protocols can be checked.

i don't really see the point in this. localhost being slower has nothing to do with the lib, and some people may be running servers on a domain in the cloud without needing to specify a port - this will get confusing. i think the current system is fine and i do not see it changing, it is the same in the other libraries too.
also, would like to say that localhost != <IP_ADDRESS> so i really do not like the idea of changing it for "performance" or whatever other reason you may have for it

The only issue I see is that localhost doesn't always map to <IP_ADDRESS>, but depends on the user configuration. Also if the user does want localhost, it probably shouldn't be opinionated on whether that should be changed to something else.
I think this is better as a note/warning in the documentation.

I don't think most libraries out there try to protect the users from themselves that much, if anything IMO you should be atleast that competent before trying to use something as complex as an asynchronous OOP based database wrapper (it might not sound like much for experienced people but boy hanging around discord python servers has taught me a lot).
Also adding a separate class for it is just adding extra unnecessary complexity to something that's extremely simple and is as I've mentioned before borderline work for any sort of library which relies on a service located at some domain.

The only issue I see is that localhost doesn't always map to <IP_ADDRESS>, but depends on the user configuration. Also if the user does want localhost, it probably shouldn't be opinionated on whether that should be changed to something else.
I think this is better as a note/warning in the documentation.

True, perhaps this should be implemented as a note/warning in the documentation

Why don't we just let the user set the IP to <IP_ADDRESS> if he/she prefers? We could add a little note in the docs saying that <IP_ADDRESS> would more performant than localhost.

Why don't we just let the user set the IP to <IP_ADDRESS> if he/she prefers? We could add a little note in the docs saying that <IP_ADDRESS> would more performant than localhost.

Yeah, that's the plan!