Weight sensor

This change is

Coverage increased (+3.4%) to 77.707% when pulling f2279a310268481a7e2a6592418d62538df1c9c2 on weight_sensor into c5c12e096a0a81823d2283e381e76a4c40ef436b on master.

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

greenpithumb/weight_sensor.py, line 3 [r2] (raw file):

import adc

_CONTAINER_WEIGHT = 100.0  #placeholder

I think this should be a parameter to the __init__ function since we might not have the same weight containers. This is another thing that should be in a config file eventually.

greenpithumb/weight_sensor.py, line 9 [r2] (raw file):

class WeightSensor(object):

This is a bit unintuitive because it's called WeightSensor but doesn't report weight. Right now, this class seems to be mixing functionality in a few ways.

It interprets what the weight sensor is saying through the ADC
It converts a weight to a volume measurement
It decides whether a water volume is too low

According to Curly's Law each class should just do one thing, so it makes sense to break this up.
We could do something like:

WeightSensor - Determines weight from an ADC
Reservoir - Determines reservoir water level, given a WeightSensor
ReservoirMonitor - Monitors a Reservoir instance and triggers an alert (sensor light, email notification, some notice in the web interface) when reservoir level is too low

greenpithumb/weight_sensor.py, line 25 [r2] (raw file):

        reservoir_weight = self._adc.read_pin(adc.PIN_WEIGHT_SENSOR)
        if not _MIN_RES_WEIGHT <= reservoir_weight <= _MAX_RES_WEIGHT:
            raise ValueError('reservoir weight out of range: %.1F' %

Does uppercase F work? I've only ever seen like %f

Comments from Reviewable

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

greenpithumb/weight_sensor.py, line 25 [r2] (raw file):
Previously, mtlynch (Michael Lynch) wrote…
> Does uppercase F work? I've only ever seen like `%f`

uppercase F does work, but I didn't mean to do that! Will switch to lowercase

Comments from Reviewable

Coverage increased (+1.5%) to 75.758% when pulling efffc278d69dfdadaf1f527d6386cf3280d5bdea on weight_sensor into c5c12e096a0a81823d2283e381e76a4c40ef436b on master.

Reviewed 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

greenpithumb/reservoir.py, line 5 [r3] (raw file):

class Reservoir(object):
    """Wrapper for a reservoir."""

Wrapper probably isn't quite the right term here. Wrapper makes sense for things like the weight sensor because there actually is a physical sensor that gives out signals and we're wrapping it with a little bit of code so that there's a Python interface to represent the signals it gives off.
Reservoir is getting a bit more abstract because there's no "reservoir sensor" that we're calling to get this value. We're creating this concept by putting together other information. (ditto for ReservoirMonitor).
Suggestion:
"""Provides information about a GreenPiThumb's system's water reservoir."""

greenpithumb/reservoir.py, line 19 [r3] (raw file):

    def reservoir_level(self):
        """Returns reservoir level in ml."""
        res_weight_grams = self._weight_sensor.reservoir_weight()

Kind of weird to abbreviate to "res" here when we're using "reservoir" everywhere else

greenpithumb/reservoir.py, line 21 [r3] (raw file):

        res_weight_grams = self._weight_sensor.reservoir_weight()
        water_weight_grams = res_weight_grams - self._container_weight
        if water_weight_grams < weight_sensor.MIN_RES_WEIGHT:

This shouldn't be necessary. It looks like WeightSensor will already throw an exception in this case so this condition should never be possible.
In general, components aren't responsible for making sure other components have returned sane results. If WeightSensor returns a value from reservoir_weight() without throwing an exception, then anything that calls that should be able to trust that unless it has more information about the context where a particular value wouldn't make sense.
Given that, I think we've actually made a mistake in splitting up this responsibility between Adc and the components that rely on Adc. Because the read_pin should never return a value outside of the (0, 1023) range, so we should push that check into read_pin and let things like MoistureSensor pass it through. Because from the perspective of MoistureSensor anything that Adc reports is valid.
But with something like LightSensor that class knows more about the pin value than Adc so it should be applying additional checking. Because an ADC pin can in general give a value of 5.0, but it never should if it's connected to a light sensor, so LightSensor should be checking for this.
So I think after this PR, we should revise the behavior of a few components so that it's like:
class Adc(object):
...
  read_pin(self, pin):
      pin_value = RPi.GPIO.read(pin)
      if not (0.0 <= pin_value <= 1023.0):
         raise ValueError('whatever')

class MoistureSensor(object):
...
  def moisture(self):
    return self._adc.read_pin(adc.PIN_MOISTURE_SENSOR)

class LightSensor(object):
...
  def get_light_level(self):
        if light_level < _LIGHT_SENSOR_MIN_VALUE:
            raise ValueError('Light sensor reading out of range')

greenpithumb/reservoir.py, line 25 [r3] (raw file):

                             water_weight_grams)

        return water_weight_grams  #1 gr water = 1 ml water

Supposably this is only at a certain temperature:
http://water.usgs.gov/edu/density.html
Should we use a multiplier of 0.99802 since that's closer to what it'll generally be indoors? If we want to go really fancy we can hook this up to TemperatureSensor  and do conversion as a function of temperature but that's probably overkill.

greenpithumb/weight_sensor.py, line 3 [r3] (raw file):

import adc

MIN_RES_WEIGHT = 0.0

If we take out the check in reservoir.py this can be private again.

greenpithumb/weight_sensor.py, line 4 [r3] (raw file):

MIN_RES_WEIGHT = 0.0
_MAX_RES_WEIGHT = 1023.0

(optional) Kinda same note about abbreviation. I try to avoid abbreviation when I can because it generally creates ambig and conf.

greenpithumb/weight_sensor.py, line 20 [r3] (raw file):

    def reservoir_weight(self):
        """Returns reservoir weight in grams."""

Now that we've split it up, this can just be a generic weight sensor. It doesn't need to know that it's weight of the reservoir.

tests/test_reservoir.py, line 21 [r3] (raw file):

        self.mock_weight_sensor.reservoir_weight.return_value = res_weight
        water_level = res.reservoir_level()
        self.assertAlmostEqual(water_level, res_weight - container_weight)

It's a bit of a red flag when our test code reimplements logic from the production code it's testing. If we made a mistake in production, we're probably making the same mistake in the test.
In this case, subtracting the container weight from the total weight is the most important thing Reservoir does, but the code is in both the production class and test class.
For test code, it's okay to introduce some redundancy for the sake of minimizing logic, so for this I'd suggest something like:
self.mock_weight_sensor.reservoir_weight.return_value = 55.0
res = reservoir.Reservoir(self.mock_weight_sensor, 50)
self.assertAlmostEqual(5.0, res.reservoir_level())

Comments from Reviewable

Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

greenpithumb/reservoir.py, line 5 [r3] (raw file):
Previously, mtlynch (Michael Lynch) wrote…
> Wrapper probably isn't quite the right term here. Wrapper makes sense for things like the weight sensor because there actually is a physical sensor that gives out signals and we're wrapping it with a little bit of code so that there's a Python interface to represent the signals it gives off.
>
> `Reservoir` is getting a bit more abstract because there's no "reservoir sensor" that we're calling to get this value. We're creating this concept by putting together other information. (ditto for `ReservoirMonitor`).
>
> Suggestion:
>
> ```python
> """Provides information about a GreenPiThumb's system's water reservoir."""
> ```
>
>

Whoops should be
"""Provides information about a GreenPiThumb system's water reservoir."""

Comments from Reviewable

greenpithumb/reservoir.py, line 21 [r3] (raw file):
Previously, mtlynch (Michael Lynch) wrote…
> This shouldn't be necessary. It looks like `WeightSensor` will already throw an exception in this case so this condition should never be possible.
>
> In general, components aren't responsible for making sure other components have returned sane results. If `WeightSensor` returns a value from `reservoir_weight()` without throwing an exception, then anything that calls that should be able to trust that unless it has *more* information about the context where a particular value wouldn't make sense.
>
> Given that, I think we've actually made a mistake in splitting up this responsibility between `Adc` and the components that rely on `Adc`. Because the `read_pin` should never return a value outside of the (0, 1023) range, so we should push that check into `read_pin` and let things like `MoistureSensor` pass it through. Because from the perspective of `MoistureSensor` anything that `Adc` reports is valid.
>
> But with something like `LightSensor` that class knows more about the pin value than `Adc` so it should be applying additional checking. Because an ADC pin can in general give a value of 5.0, but it never should if it's connected to a light sensor, so `LightSensor` should be checking for this.
>
> So I think after this PR, we should revise the behavior of a few components so that it's like:
>
> ```python
> class Adc(object):
> ...
>   read_pin(self, pin):
>       pin_value = RPi.GPIO.read(pin)
>       if not (0.0 <= pin_value <= 1023.0):
>          raise ValueError('whatever')
>
> class MoistureSensor(object):
> ...
>   def moisture(self):
>     return self._adc.read_pin(adc.PIN_MOISTURE_SENSOR)
>
> class LightSensor(object):
> ...
>   def get_light_level(self):
>         if light_level < _LIGHT_SENSOR_MIN_VALUE:
>             raise ValueError('Light sensor reading out of range')
> ```

Good points! Agreed that we should revise ```Adc``` and the other components as necessary.  In this specific case, though, we still need to do a check because  we have the extra information of the container weight to factor in. So the weight reading could be in range, but if it's less than the weight of the container we still want to raise an exception.

Comments from Reviewable

Review status: all files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

greenpithumb/reservoir.py, line 25 [r3] (raw file):
Previously, mtlynch (Michael Lynch) wrote…
> Supposably this is only at a certain temperature:
>
> http://water.usgs.gov/edu/density.html
>
> Should we use a multiplier of `0.99802` since that's closer to what it'll generally be indoors? If we want to go really fancy we can hook this up to `TemperatureSensor`  and do conversion as a function of temperature but that's probably overkill.

Oh interesting, that makes sense. I think in this instance it's fine to approximate a 1:1 relationship since precise values don't offer any practical benefit. I'm happy to change it though if you disagree. For the time-being I just updated the comment to reflect that 1 gr of water is roughly equal to 1 ml of water.

greenpithumb/weight_sensor.py, line 3 [r2] (raw file):
Previously, mtlynch (Michael Lynch) wrote…
> I think this should be a parameter to the `__init__` function since we might not have the same weight containers. This is another thing that should be in a config file eventually.

Done.

greenpithumb/weight_sensor.py, line 25 [r2] (raw file):
Previously, JeetShetty wrote…
> uppercase F does work, but I didn't mean to do that! Will switch to lowercase

Done.

Comments from Reviewable

Coverage increased (+1.5%) to 75.758% when pulling 29913829ddecea628fa160d47a3a88088888827e on weight_sensor into c5c12e096a0a81823d2283e381e76a4c40ef436b on master.

Review status: 1 of 6 files reviewed at latest revision, 2 unresolved discussions.

greenpithumb/reservoir.py, line 21 [r3] (raw file):
Ah, yeah you're right about the container weight being the min weight that the reservoir should ever see.
In that case, weight_sensor.MIN_WEIGHT doesn't need to exist because it'll just be the same as  the generic check >= 0 check in read_pin. Then I think we should just do something like
reservoir_weight_grams = self._weight_sensor.weight()
if reservoir_weight_grams < self._reservoir_weight:
  raise ReservoirUnderWeightException(
    'Unexpected weight measured for reservoir (%.1f g), as it is less than the weight of an empty reservoir (%.1f g).' % (
    reservoir_weight_grams, self._reservoir_weight))

I think we want a custom exception because this doesn't seem to fit the definition of a ValueError (we're probably stretching it in other places as well):

Raised when a built-in operation or function receives an argument that has the right type but an inappropriate value,

https://docs.python.org/2/library/exceptions.html#exceptions.ValueError

greenpithumb/reservoir.py, line 25 [r3] (raw file):
Previously, JeetShetty wrote…
> Oh interesting, that makes sense. I think in this instance it's fine to approximate a 1:1 relationship since precise values don't offer any practical benefit. I'm happy to change it though if you disagree. For the time-being I just updated the comment to reflect that 1 gr of water is roughly equal to 1 ml of water.

Yeah that makes sense. 1:1 approximation sounds good to me.

Comments from Reviewable

Reviewed 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

Review status: all files reviewed at latest revision, 2 unresolved discussions.

greenpithumb/reservoir.py, line 21 [r3] (raw file):
Previously, mtlynch (Michael Lynch) wrote…
> Ah, yeah you're right about the container weight being the min weight that the reservoir should ever see.
>
> In that case, `weight_sensor.MIN_WEIGHT` doesn't need to exist because it'll just be the same as  the generic check >= 0 check in `read_pin`. Then I think we should just do something like
>
> ```python
> reservoir_weight_grams = self._weight_sensor.weight()
> if reservoir_weight_grams < self._reservoir_weight:
>   raise ReservoirUnderWeightException(
>     'Unexpected weight measured for reservoir (%.1f g), as it is less than the weight of an empty reservoir (%.1f g).' % (
>     reservoir_weight_grams, self._reservoir_weight))
> ```
>
> I think we want a custom exception because this doesn't seem to fit the definition of a `ValueError` (we're probably stretching it in other places as well):
>
> >Raised when a built-in operation or function receives an argument that has the right type but an inappropriate value,
>
> https://docs.python.org/2/library/exceptions.html#exceptions.ValueError

For the above change I guess the best thing to do is just to merge this PR as is, and then implement that change in another PR where we do the checking in ```read_pin``` and then update the rest of the classes to reflect that change?
Oh I didn't realize that was the use for ValueError! What's the reason for allowing it to be manually raised in that case?

Comments from Reviewable

Review status: all files reviewed at latest revision, 2 unresolved discussions.

greenpithumb/reservoir.py, line 21 [r3] (raw file):

For the above change I guess the best thing to do is just to merge this PR as is, and then implement that change in another PR where we do the checking in read_pin and then update the rest of the classes to reflect that change?

Yeah that sounds good.

Oh I didn't realize that was the use for ValueError! What's the reason for allowing it to be manually raised in that case?

Python is super permissive, so it will allow us to do pretty much anything.
I can't find documentation to back me up, but I think it's accepted practice to use ValueError when it's basically exposing a programming error and the context is the same as when the built-in libraries raise ValueError.
So something like in clock.py makes sense:
        if wait_time_seconds < 0.0:
            raise ValueError('Wait time cannot be negative: %f' %
                             wait_time_seconds)

Because it can probably only happen if we screw something up in the code and call it wrong.
Whereas checking the result of something else we're calling isn't so clearly a programming error. The code might actually be doing the right thing, but the system is in a bad state, so it makes sense to have a custom exception for those kinds of cases.

Comments from Reviewable


docs: fix contributing link

Thank you for your help, but it looks like we've already fixed this.
If I'm wrong, please re-open this issue and we'll get it sorted out. Thanks!


release: 1.3.1

:robot: I have created a release beep boop
1.3.1 (2022-12-13)
Features

deps: bump dependencies (ce741b4)

This PR was generated with Release Please. See documentation.

:robot: Release is at https://github.com/starudream/douyu-task/releases/tag/v1.3.1 :sunflower:


cmd/cgo: do not let Go pointers end up in C

We need some way to prevent Go pointers from crossing the boundary into C.

People write code like (calling a C function that takes a T* to fill in).

var x C.T
C.f(&x)

One option is to disallow this - a Go pointer &x is being passed to C.

Another option is to have cgo's wrapper for C.f do a shallow copy into an allocated C
pointer, then run f, then copy the data back into the original Go data, then free the
allocated C pointer, then return.

If the second option handles 99% of the pointers being passed, then we probably should
do it. On the other hand, the free might create a dangling pointer that could confuse
people. At least it's a dangling pointer into the C heap and not the Go heap. :-)

See also #12116 .

Proposal at #12416 .

Proposal #12416 was implemented.  Closing this issue.


ProTanki protocol

Hey @Assasans,
I'm not sure if you're still interested in this project or if you have moved on.
Nevertheless you wrote that you have a quite some trouble with the protocol after the protocol update.
I've attached you a dump of all packet ids, their fields as well as the model ids they belong to.
Additionally I added my own list of human friendly names for easy understanding (please note that this list is not complete as I mainly created  it for my own purposes).
If you're interested I can share more details about the protocol so you can easily implement the new one.
Except for the encoding (and a few adjustments here and there) I haven't encountered any big changes.
pt-encryption-context.as.txt
human-packet-names.txt
pt-scpacker-packets.txt
Regards
Markus

BTW a fully working protocol implementation:
https://github.com/WolverinDEV/protanki-protocol-rs

Hello, I glanced over packets and am interested in further development.
I'd like to rewrite the server in Rust because I don't have enough computational resources for JVM-targeted development right now.
I also saw DaniloPalmeira/protanki-server, which works with new protocol.
Thank you for creating definitions for all packets.

Well running rust analyser is most likely way heavier then spinning up a JVM 🤕
But I do understand your reasoning.
It's interesting how many starting approaches exist writing the server from scratch :D
I'll probably keep the protocol files updated on the protanki-protocol-rs repo and complete documenting the yet unknown packet ids.
BTW do you know a little bit about the history of pro tanki and so on?
I'm pretty new to all this kind of stuff. (PS: I'm also in your DC)

I know only the fact that ProTanki are made by the same developers as GTanks, which were closed 1 year after release of ProTanki.
I made a chart of Tanks projects earlier (grayed out - closed / unmaintained):

Ahh that's a pretty need chart. Are you sure that GTanks is a reimplementation of the original TO or just their source updated?
As well why did GTanks close?

As well why did GTanks close?

Their online was at very low spots, with often times where there was 0 players online.
P.S they didn't care anymore because of their success with ProTanki.

Ahh reasonable. So ProTanki is just the more popular version of GTanks, right?

The newer version of tanki, people are not as intrigued with very old tanki nowadays.

Can you estimate how popular ProTanki is?

3000 active players a day

Hui that's ~50 % of the regular Tanks player base

I have 2018 version of GTanks' client & server and it uses same protocol as ProTanki before update (why would they use hacky text protocol if original TO always used binary protocol).
Also, the GTanks codebase is a lot smaller than official Tanki Online's.

Decompiled server code

As well why did GTanks close?

There were too few new players. When you reached one of the first ranks you had battles with only high-rank players that kept constantly killing low-ranks, though they gave away crystal drop you still wouldn't be able to rank up.

BTW a fully working protocol implementation: https://github.com/WolverinDEV/protanki-protocol-rs

By the way, may I use your code in this repository under MIT / Apache 2.0 dual license?

@Assasans the argument with the protocol is pretty reasonable.
The more I dig trough the more easy does it seem to create a pretty simple TO server.
As far as I can the, the hardest would be implementing an anticheat (which PT has btw.).
@Assasans would you mind sharing the old gtanks server with me?
As PT and GTanks are developed by the same person? there might be some insights of what measures they take to enforce "proper" game play.

@Assasans the argument with the protocol is pretty reasonable.
The more I dig trough the more easy does it seem to create a pretty simple TO server.
As far as I can the, the hardest would be implementing an anticheat (which PT has btw.).
@Assasans would you mind sharing the old gtanks server with me?
As PT and GTanks are developed by the same person? there might be some insights of what measures they take to enforce "proper" game play.

the protanki anticheat is very poorly made

By the way, may I use your code in this repository under MIT / Apache 2.0 dual license?

Used MIT.
If I may ask, what are your intensions?

@Assasans would you mind sharing the old gtanks server with me?

https://drive.google.com/file/d/1rWv-D_kfZeTnxPOdYrDjI2viLrwT4F4o/view
But they did some updates to ProTanki anticheat after we raided them with my patched client.

Used MIT. If I may ask, what are your intensions?

If you're about licensing - just because it's common to the Rust ecosystem (though I always used MIT only)
If you're about the code itself - to have all core components in one place and easily editable (bypassing PRs, especially if the change is relevant for this server only).

Ahh thank you :)

to have all core components in one place and easily editable

You can just create a fork :)

GTanks 2018
Good afternoon. Could you share this source code for the server and client?

@Assasans would you mind sharing the old gtanks server with me?

https://drive.google.com/file/d/1rWv-D_kfZeTnxPOdYrDjI2viLrwT4F4o/view But they did some updates to ProTanki anticheat after we raided them with my patched client.

Used MIT. If I may ask, what are your intensions?

If you're about licensing - just because it's common to the Rust ecosystem (though I always used MIT only) If you're about the code itself - to have all core components in one place and easily editable (bypassing PRs, especially if the change is relevant for this server only).

Can you re-upload the file, please
Можешь перезалить файл? Пожалуйста


python37Packages.datadog: 0.39.0 -> 0.40.0

Automatic update generated by nixpkgs-update tools. This update was made based on information from passthru.updateScript.
meta.description for python37Packages.datadog is: "The Datadog Python library"
meta.homepage for python37Packages.datadog is: "https://github.com/DataDog/datadogpy"
meta.changelog for python37Packages.datadog is: ""
Updates performed

Ran passthru.UpdateScript

To inspect upstream changes
Impact

Checks done (click to expand)

built on NixOS

Warning: a test defined in passthru.tests did not pass

0 of 0 passed binary check by having a zero exit code.

0 of 0 passed binary check by having the new version present in output.

found 0.40.0 with grep in /nix/store/0rfrjifwbx3xj5d2rwqin0m0f8vigb42-python3.7-datadog-0.40.0

directory tree listing: https://gist.github.com/5d1e19d372c79b8e01507d0485884001

du listing: https://gist.github.com/12e01ec225089824d0cfbc0cd31d3a67

Rebuild report (if merged into master) (click to expand)

37 total rebuild path(s)

10 package rebuild(s)

9 x86_64-linux rebuild(s)
9 i686-linux rebuild(s)
10 x86_64-darwin rebuild(s)
9 aarch64-linux rebuild(s)

First fifty rebuilds by attrpath
nixops
python37Packages.datadog
python37Packages.gradient
python37Packages.gradient_statsd
python38Packages.datadog
python38Packages.gradient
python38Packages.gradient_statsd
python39Packages.datadog
python39Packages.gradient
python39Packages.gradient_statsd

Instructions to test this update (click to expand)

Either download from Cachix:
nix-store -r /nix/store/0rfrjifwbx3xj5d2rwqin0m0f8vigb42-python3.7-datadog-0.40.0 \
  --option binary-caches 'https://cache.nixos.org/ https://nix-community.cachix.org/' \
  --option trusted-public-keys '
  nix-community.cachix.org-1:mB9FSh9qf2dCimDSUo8Zy7bkq5CX+/rkCWyvRCYg3Fs=
  cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY=
  '

(The Cachix cache is only trusted for this store-path realization.)
For the Cachix download to work, your user must be in the trusted-users list or you can use sudo since root is effectively trusted.
Or, build yourself:
nix-build -A python37Packages.datadog https://github.com/r-ryantm/nixpkgs/archive/444ed9d8e9b8d61d7d34455d9c09fb3e22de20fa.tar.gz

After you've downloaded or built it, look at the files and if there are any, run the binaries:
ls -la /nix/store/0rfrjifwbx3xj5d2rwqin0m0f8vigb42-python3.7-datadog-0.40.0
ls -la /nix/store/0rfrjifwbx3xj5d2rwqin0m0f8vigb42-python3.7-datadog-0.40.0/bin

Pre-merge build results
We have automatically built all packages that will get rebuilt due to
this change.
This gives evidence on whether the upgrade will break dependent packages.
Note sometimes packages show up as failed to build independent of the
change, simply because they are already broken on the target branch.
  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/latest/m$
    PytestUnknownMarkWarning,

@lzorkin Python 2 isn't really supported anymore. Is there a leaf package that depends on python2Packages.datadog?

@dotlambda python2Packages.datadog needed to nixops 1.7.

@Izorkin fixed by https://github.com/NixOS/nixpkgs/pull/119316


Update delete-a-push-subscription.md

This doesn't work if you connect to subscriber - have to connect to the publisher.
(Guessing this is true for pull subscribers too but haven't got there yet)

@MashaMSFT

Can you review this PR?
IMPORTANT: When this content is ready to merge, you must add #sign-off in a comment or the approval may get overlooked.

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

Perfect, thanks @jpomfret ! #sign-off


Player can join game son mobile

This PR closes the following user story: https://trello.com/c/XRgyXl95/35-5-as-robert-i-want-to-play-even-when-philippe-is-not-nearby
Depends on #30

Can you add a screenshot?

Fait!


Status of import is pending

When I import a .csv, the status of the import in the history tab is pending. Also, there are no rows added to the the database. We tried the develop branch but this didn't solve it.
The task hud says Importing filename.csv and it seems like it keeps hanging while performing this task.

I’ve just installed a completely fresh copy of Craft (version 2.3.2625) and the import plugin and everything seems to be working fine. Nothing has changed in my development environment, except perhaps Craft has been updated since I last tackled this, so I’m unable to provide any tips other than try again if you are experiencing this problem.

It's really unreproducable for me too. The useEmailAsUsername issue is another case tho.


Only allow moderators to start new topic

On certain streams, for organisational purposes it is convenient to maintain a fixed set of topics. Rather than asking users to keep in mind the list of allowable topics when writing a message, or requiring moderators to clean up by moving the message to the right topic, it would be good to have a per-stream setting on who is allowed to start a new topic. Non-mod users should still be able to reply to existing topics, but not start new ones.

Thanks for the suggestion, @lzmartinico ! Closing as a duplicate of #19642. If you'd like to post a comment there describing your use case, that would help us prioritize this feature.


NIFI-11749 Upgrade Bouncy Castle from 1.74 to 1.75

Summary
NIFI-11749 Upgrades Bouncy Castle from 1.74 to 1.75, incorporating a minor bug fix for TLS renegotiation.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking

[X] Apache NiFi Jira issue created

Pull Request Tracking

[X] Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
[X] Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

[X] Pull Request based on current revision of the main branch
[X] Pull Request refers to a feature branch with one commit containing changes

Verification
Please indicate the verification steps performed prior to pull request creation.
Build

[X] Build completed using mvn clean install -P contrib-check

[X] JDK 17

Licensing

[ ] New dependencies are compatible with the Apache License 2.0 according to the License Policy
[ ] New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

[ ] Documentation formatting appears as expected in rendered files

+1 LGTM, thanks for the upgrade! Merging to support/nifi-1.x and main


PLEASE HELP. I DID A BIG MISTAKE.

I WAS HAVING PROBLEMS WITH THE SUBMIT, SO I CHANGED TO ANOTHER WALLET TO TRY IF IT WORKS.
I REALIZED THE SUBMIT AND PAY THE ENTIRE FUND BUT I FORGOT TO CHANGE MY VIDEO WITH THE NEW WALLET!!!!
ITS THERE ANYWAY THAT WE CAN SOLVE IT?? I DONT WANT TO LOSE MY MONEY!!

No shouting.


Use "official" faster-whisper models

Hi,
Would it be possible for this project to use the same models that are downloaded by faster-whisper? See the available models here: https://huggingface.co/guillaumekln
Using the same models would ensure consistency between this project and faster-whisper. For example your models do not seem to contain a recent fix to the configuration:
https://huggingface.co/guillaumekln/faster-whisper-tiny.en/commit/7d45cf02c1ed72d240c0dbf99d544d19bef1b5a3
https://huggingface.co/guillaumekln/faster-whisper-tiny/commit/518d6e0b5a068b278f66842b17377f9523de5cd1
(all models were updated similarly)

Hello
Yes, this makes sense. I built by own model download system before faster-whisper was providing the functionality.
The only thing that I need to be able to do it right now is a new faster-whisper version that includes this change:
https://github.com/guillaumekln/faster-whisper/commit/2b53dee6b697ba1364d7ba1b1c1ce3a6a2dc05df, since users in whisper-ctranslate2 can specify the directory what they want to use as cache. If you can do a new faster-whisper release it will be great, thanks

I just released a new version 0.5.0.

We use now the official models. Thanks for your support improving model loading in faster-whisper


Trouble with ui-router customization

Hello guys!
I am in trouble with a specific requirement here for our Application.
We a are setting-up an angular application inside a pre-existent Rails Application.
As a full refactor of our code-base is currently out-of-the-question we are dealing with some hard customization and hackings on both sides to allow a incremental introduction of Angular.
What we need for now is a way to tell the ui-router to bind only to the links we have the ng-sref attribute and do not bother with all the regular href links within our app.
Is there a way to achieve this behavior ?
Thank you very much in advance.
Best,
Moacir

This is a core angular feature that UI-Router uses.   When clicking a link, core angular triggers a $locationChangeStart event which UI-Router responds to.  According to the $location docs this can be customized.
I assume you're using html5Mode?
Try $locationProvider.html5Mode({ enabled: true, rewriteLinks: false });
http://plnkr.co/edit/f8iql3wI3Q5P8JkcvUy8?p=info


Trees with linear models at leaves

Implements boosting for trees with linear models at the leaves (sometimes called M5 trees). This is a hybrid between traditional tree boosting and the model proposed in the paper Gradient Boosting with Piece-Wise Linear Regression Trees by Shi, Li, and Li (https://arxiv.org/pdf/1802.05640.pdf), which is mentioned in #1315. In this PR, the tree structure is created by finding the best split in the normal way, but then we calculate a linear model on each leaf. In contrast, in the paper, the splits are chosen by calculating the linear models for each potential split point, which is much more computationally intensive and would require more significant code changes in LightGBM. (The paper above actually mentions M5 trees, in Appendix D, but only tests existing slow implementations, which give poor results compared to their code. I think with the better implementation from this PR, M5 trees would come close to the performance of the fully-linear approach.)
The running time of the linear-leaf model is around 10-20% more than traditional tree boosting (depending on the dataset etc), but it converges faster, so overall it gives a small improvement in training time (and also can achieve slightly better accuracy). Memory use is higher since we need so store the full feature data.
Regularisation can be controlled with the parameter linear_lambda; this is important because it's more prone to overfitting than the traditional tree-boosting model. Also it's important to scale the data before training so that all features have similar mean and standard deviation.
The code uses parts of the Eigen library licensed under MPL2.
I have only implemented this for Python. I think to get it working for R it would require some changes to the data loading interface, but I'm not very familiar with R, so maybe someone else would like to take that on.
Here is a test script to measure performance on the SUSY physics data (https://archive.ics.uci.edu/ml/datasets/SUSY).
import pandas as pd
import numpy as np
import lightgbm as lgb
import time as time
from sklearn.metrics import log_loss, roc_auc_score
import matplotlib.pyplot as plt

# read data
train = pd.read_csv('susy.csv', header=None)
np.random.seed(0)

# normalise
for i in range(len(train.columns) - 1):
    m, s = train.iloc[:, i + 1].agg(['mean', 'std'])
    train.iloc[:, i + 1] = (train.iloc[:, i + 1] - m) / s

train_bool = train.index <= train.index.max() - 500000  # last 500000 is the hold-out test set
valid_bool = train.index <= 500000  # validation set for early stopping

# linear model
t = time.time()
lgb_train = lgb.Dataset(train.loc[train_bool & ~valid_bool, list(range(1, 19))], label=train.loc[train_bool & ~valid_bool, 0])
lgb_test = lgb.Dataset(train.loc[valid_bool, list(range(1, 19))], label=train.loc[valid_bool, 0])
valid_sets = [lgb_train, lgb_test]
valid_names = ['train', 'valid']
# linear model
params = {'objective': 'binary', 'seed': 0, 'num_leaves': 16, 'learning_rate': 0.05,
          'metric': 'binary_logloss', 'verbose': 2, 'linear_lambda': 0.01, 'linear_tree': True}
res = {}
t0 = time.time()
time_arr_linear = []
timer_callback = lambda env: time_arr_linear.append(time.time() - t0)
est_linear = lgb.train(params, lgb_train, num_boost_round=500,valid_sets=valid_sets, valid_names=valid_names,
                       categorical_feature=[], evals_result=res, callbacks=[timer_callback])
linear_time = time.time() - t
print("Linear model")
print("training time ", linear_time)

# normal model
params = {'objective': 'binary', 'seed': 0, 'num_leaves': 16, 'learning_rate': 0.05,
          'metric': 'binary_logloss', 'verbose': 2, 'linear_tree': False}
t = time.time()
res2 = {}
lgb_train = lgb.Dataset(train.loc[train_bool & ~valid_bool, list(range(1, 19))], label=train.loc[train_bool & ~valid_bool, 0])
lgb_test = lgb.Dataset(train.loc[valid_bool, list(range(1, 19))], label=train.loc[valid_bool, 0])
valid_sets = [lgb_train, lgb_test]
valid_names = ['train', 'valid']
t0 = time.time()
time_arr_nonlinear = []
timer_callback = lambda env: time_arr_nonlinear.append(time.time() - t0)
est = lgb.train(params, lgb_train, valid_sets=valid_sets, valid_names=valid_names, num_boost_round=500,
                categorical_feature=[], evals_result=res2, callbacks=[timer_callback])
nonlinear_time = time.time() - t

# plot loss vs iterations
plt.figure()
plt.plot(res['train']['binary_logloss'])
plt.plot(res['valid']['binary_logloss'])
plt.plot(res2['train']['binary_logloss'])
plt.plot(res2['valid']['binary_logloss'])
plt.legend(['train_linear', 'test_linear', 'train', 'test'])

# plot loss vs time
plt.figure()
plt.plot(time_arr_linear, res['train']['binary_logloss'])
plt.plot(time_arr_linear, res['valid']['binary_logloss'])
plt.plot(time_arr_nonlinear, res2['train']['binary_logloss'])
plt.plot(time_arr_nonlinear, res2['valid']['binary_logloss'])
plt.legend(['train_linear', 'test_linear', 'train', 'test'])

# evaluate performance on hold-out set
t = time.time()
p_linear = est_linear.predict(train.loc[~train_bool, list(range(1, 19))], num_boost_round=est_linear.best_iteration)
linear_pred_time = time.time() - t
print("Linear model")
print("training time ", linear_time)
print("prediction time ", linear_pred_time)
print("log loss ", log_loss(train.loc[~train_bool, 0], p_linear))
print("auc ", roc_auc_score(train.loc[~train_bool, 0], p_linear))

t = time.time()
p = est.predict(train.loc[~train_bool, list(range(1, 19))], num_boost_round=est.best_iteration)
nonlinear_pred_time = time.time() - t
print("\n\nConstant model")
print("training time ", nonlinear_time)
print("prediction time ", nonlinear_pred_time)
print("log loss ", log_loss(train.loc[~train_bool, 0], p))
print("auc ", roc_auc_score(train.loc[~train_bool, 0], p))

plt.title('Accuracy vs time on SUSY dataset (500 iterations)', fontsize=12)
plt.xlabel('Time (seconds)', fontsize=12)
plt.ylabel('Log loss', fontsize=12)

plt.figure()
plt.scatter(p, p_linear)
plt.xlim([0, 0.2])
plt.ylim([0, 0.2])

Output:
Linear model
training time<PHONE_NUMBER>8962097
prediction time  3.5460383892059326
log loss  0.4245472152015672
auc  0.8775945941602206

Constant model
training time  89.05728363990784
prediction time  2.811807155609131
log loss  0.4258133037378132
auc  0.8769452618339838
Out[3]: (0, 0.2)

Full training graph:

Training graph zoomed in on y-axis to show different convergence:

@btrotta
Thank you so much!
BTW, it seems there are lots of Eigen codes, maybe it is better to include it as submodule?

also cc @shiyu1994 for the GBDT-PL

It seems R cmd check is finding some problems with the Eigen files:
File which contains pragma(s) suppressing important diagnostics
  ‘src/eigen/Eigen/src/Core/arch/SSE/Complex.h’
Files which contain pragma(s) suppressing diagnostics:
  ‘src/eigen/Eigen/src/Core/util/DisableStupidWarnings.h’
  ‘src/eigen/test/AnnoyingScalar.h’ ‘src/eigen/test/indexed_view.cpp’
  ‘src/eigen/test/packetmath_test_shared.h’
  ‘src/eigen/test/symbolic_index.cpp’
  ‘src/eigen/test/vectorization_logic.cpp

Since I haven't implemented the R interface anyway, I have used the preprocessor directive LGB_R_BUILD to avoid compiling the code which requires Eigen. We would need to revisit this if/when someone implements the R interface.

@btrotta thanks very much! is this ready to review?

@guolinke Yes, ready for review

Thanks @guolinke. I'm still working through your comments but have fixed some of the easier ones as noted above.

@btrotta BTW, will this affect the training speed when disabling linear_tree?

@btrotta BTW, will this affect the training speed when disabling linear_tree?

@guolinke No, we don't do any extra calculation unless linear_tree is True. I just checked this by training 5 times in the script above and averaging. Timing below:
Old version:<PHONE_NUMBER>9745179 seconds
Code from this PR with linear_tree=False:<PHONE_NUMBER>6823731

ping @shiyu1994 for another round of review.

@jameslamb @btrotta
BTW, is this feature disabled in R ? can this pass the cran checks?

BTW, is this feature disabled in R ? can this pass the cran checks?

Yes, in the R package if a user tries to use this feature they will get a fatal error. I'm not sure about the CRAN requirements but all the CI checks are passing currently.

This linear model can support linear regression ？

I think we should merge this before 3.2.0, ping @shiyu1994 for another round for review.

This PR is ready for merge. @btrotta Could you resolve the conflicts? And there're two more suggestions in linear_tree_learner.cpp to use multi-threading. Thanks a lot.

This linear model can support linear regression ？

From the code, it supports regression, except regression with L1 loss.

@shiyu1994 Thanks for the detailed review! I've merged the changes from master and updated as per your comments.

@shiyu1994 you can approve this PR if there are no problems.

Tuned the PR on small subsets of our benchmark datasets. Following shows the results. Linear leaves do improve the results on some datasets. Since these datasets are small, we expect linear models to overfit the data easier. And the features are not normalized, this can also affect the accuracy of linear leaves.
No bug is reported in all the tuned cases, whether use linear models or not. I'll merge this PR.

leaf type
higgs
yahoo
msltr
dataexpo
allstate
year

-
AUC
NDCG@5
NDCG@5
AUC
AUC
RMSE

constant
0.820457±0.000496
0.726132±0.002645
0.427234±0.006895
0.717934±0.001222
0.537855±0.017714
8.832463±0.012105

linear
0.822166±0.000633
0.722051±0.001225
0.430325±0.004525
0.716811±0.000925
0.535730±0.015776
8.816403±0.010700

is linear_tree available with the 3.1.1 version of the python API?

is linear_tree available with the 3.1.1 version of the python API?

@StrikerRUS @guolinke IMO this pull introduced a bug or unexpected behavior in the class method SizesInByte.  It no long returns the data_.size() but the AlignedSize(data_.size()).  This is BAD if you are using it for allocation or copying from a desire location in the data.  In the past, SizesInByte was NEVER BIGGER than num_data_ but NOW in some case it is.  There should be some way of getting the original data_.size().

@StrikerRUS @guolinke IMO this pull introduced a bug or unexpected behavior in the class method SizesInByte.  It no long returns the data_.size() but the AlignedSize(data_.size()).  This is BAD if you are using it for allocation or copying from a desire location in the data.  In the past, SizesInByte was NEVER BIGGER than num_data_ but NOW in some case it is.  There should be some way of getting the original data_.size().

@ChipKerchner It seems that the AlignedSize(data_.size()) in SizesInByte is not introduced by this PR, but #3415

@ChipKerchner It seems that the AlignedSize(data_.size()) in SizesInByte is not introduced by this PR, but #3415

@shiyu1994 @guolinke
Related to the AlignedSize problem: https://github.com/microsoft/LightGBM/issues/3450#issuecomment-758583425.

@shiyu1994 @guolinke
Related to the AlignedSize problem: https://github.com/microsoft/LightGBM/issues/3450#issuecomment-758583425.

When will this go to pip please? Really interested to give this a try!

The next formal release is being tracked in #3872 , you can subscribe to that for updates. We cannot give an exact date at this time, but you can see a list there of the work that still needs to be done.

@JoshuaC3

When will this go to pip please? Really interested to give this a try!

For now please feel free to install from nightly wheels.
https://lightgbm.readthedocs.io/en/latest/Installation-Guide.html

@StrikerRUS I only looked at the git pip install on the pip website itself. This broke because "Windows build issues". I will give the Wheels a spin!!
Also, @jameslamb, I recognised you name but wasn't sure where from. I've just realised!! recent-developments-in-lightgbm. Excellent video so thanks. Hopefully we get another one with 4.0 😉🤞
Thank both/all 👏

Hi @btrotta, thanks for your work! Is it possible to access the coefficients and offsets of the linear models at each leaf? I did not find this information in e.g. the dump_model() method.

Hi @spiralulam, thanks for using LightGBM. If you dump the tree model with linear_tree enabled, you should see an entry named leaf_coeff in the part of each tree. That's the coefficients of the linear leaves.

Hi @spiralulam, thanks for using LightGBM. If you dump the tree model with linear_tree enabled, you should see an entry named leaf_coeff in the part of each tree. That's the coefficients of the linear leaves.

Thanks for the answer. Does that mean I basically have to parse a string to obtain these information using save_model()? The dump_model() method which returns a dictionary does not contain leaf_coeff nor leaf_const.

Just checked the method that dumps a tree to JSON. Unfortunately, information of linear leaves are not handled. So currently parsing from the model text file seems to be the only solution. Sorry for the inconvenient.
This should be a very useful case when using linear trees, and I believe we should provide direct access to linear model coefficients through C++, Python and R API.

That would be awesome, indeed.

Hi @btrotta, Thx for your work! Does the code allow using a subset of the features for the tree splits and another totally different to estimate the linear model at each leaf?

@cc22226 Thanks for using LightGBM. Currently the linear models at leaves will consider all the numerical features (or non-categorical features). And there's no parameter to control which features are used in the linear models and which are used in the splits. I think it would be nice sometimes to have these two parts of features being separated. And maybe we can leave that as a feature request.

Thanks for your response.
Cesar
On Thu, Jun 17, 2021 at 11:49 PM shiyu1994 @.***> wrote:

@cc22226 https://github.com/cc22226 Thanks for using LightGBM.
Currently the linear models at leaves will consider all the numerical
features (or non-categorical features). And there's no parameter to control
which features are used in the linear models and which are used in the
splits. I think it would be nice sometimes to have these two parts of
features being separated. And maybe we can leave that as a feature request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/microsoft/LightGBM/pull/3299#issuecomment-863734052,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ANQTDQEMNLEGE673A7OWED3TTK66LANCNFSM4P4HS22Q
.


Finish Dim.Auto (Content)

This puppy is not really finished:

https://github.com/gui-cs/Terminal.Gui/pull/3451

DimAutoStyle.Content sorta works. Needs moar. This issue is to track.

I appreciate you doing this, but there is a LOT going on in those tests. Is there any way you can simplify them down to more precise cases? If not, that's ok... but it would make my life a lot easier.

Basically in each test, using DimAutoStyle.Auto, I am testing 3 views, where the first does not use the maximum value, the second uses an absolute value and the third uses Fill. In the first test I am using ContentSize and in the second I am using Text, where view2.TextDirection = TextDirection.TopBottom_LeftRight. Practically the tests pass as expected and the only exception is in the second test, using Text, in the view that is using Fill, it does not respect the width of the superview (40), returning Text.Length (35). I didn't want to use Theory to avoid complicating things and making it simpler.


Update upickle to 1.3.7

Updates com.lihaoyi:upickle from 1.3.4 to 1.3.7.
GitHub Release Notes - Version Diff
I'll automatically update this PR to resolve conflicts as long as you don't change it yourself.
If you'd like to skip this version, you can just close this PR. If you have any feedback, just mention me in the comments below.
Configure Scala Steward for your repository with a .scala-steward.conf file.
Have a fantastic day writing Scala!

Ignore future updates
Add this to your .scala-steward.conf file to ignore future updates of this dependency:
updates.ignore = [ { groupId = "com.lihaoyi", artifactId = "upickle" } ]

labels: library-update, semver-patch

Superseded by #1228.


can we get an npm compat update for node v6

If you make a PR, I will be happy to merge it, but I don't plan to add node v6 support since it's an Angular 1.x plugin.


Don't require /packit build by user with write permissions

Since Packit creates separate projects for each PR IIUC, it is not necessary
to - at least from Copr POV - require maintainers to trigger the build.  We
can simply start them.

TBH, I am not 100% sure if I get this right.

Since Packit creates separate projects for each PR

Yes, by-default packit is the owner of that Copr project.

it is not necessary to - at least from Copr POV - require maintainers to trigger the build.
You mean GitHub project maintainer, don't you?

If the user or organisation is approved (and it is I think) and user has merge permission in that project, we trigger the build automatically. (I think the write access is the one we are speaking about, maintain is higher than write.)
If this does not work, there might be some bug during the process and we should fix it.

TBH, I am not 100% sure if I get this right.

Since Packit creates separate projects for each PR

Yes, by-default packit is the owner of that Copr project.

it is not necessary to - at least from Copr POV - require maintainers to trigger the build.
You mean GitHub project maintainer, don't you?

If the user or organisation is approved (and it is I think) and user has merge permission in that project, we trigger the build automatically. (I think the write access is the one we are speaking about, maintain is higher than write.)
If this does not work, there might be some bug during the process and we should fix it.

this would make things so much easier on our side
@praiskup if you are fine with such behaviour, as the COPR project lead, I'm pretty sure we can deliver :)

this would make things so much easier on our side
@praiskup if you are fine with such behaviour, as the COPR project lead, I'm pretty sure we can deliver :)

I think the write access is the one we are speaking about, maintain is higher than write

That's what I meant ... from Copr perspective, you don't need to require write
permissions as the users can not make any damage.  The only thing that user
can affect is the build itself, or crack the concrete builder VM.  The VMs are
reused for other builds, but only for buidls in the same project (and as each
PR means separate Copr project, it is just fine).

this would make things so much easier on our side

Packit would be actually even more convenient to use.

I think the write access is the one we are speaking about, maintain is higher than write

That's what I meant ... from Copr perspective, you don't need to require write
permissions as the users can not make any damage.  The only thing that user
can affect is the build itself, or crack the concrete builder VM.  The VMs are
reused for other builds, but only for buidls in the same project (and as each
PR means separate Copr project, it is just fine).

this would make things so much easier on our side

Packit would be actually even more convenient to use.

@praiskup Thanks for the explanation.
Can we get rid of the whitelist as well then? (I think you discuss this with @TomasTomecek  sometime ago.)

@praiskup Thanks for the explanation.
Can we get rid of the whitelist as well then? (I think you discuss this with @TomasTomecek  sometime ago.)

I'm not sure what do you mean by whitelist in this context.

I'm not sure what do you mean by whitelist in this context.

OK, we have two-step verification:

Allow namespace/user to build in Copr in general (=whitelist).
Allow PR author to build in that specific project (required write permission in that project).

OK, we have two-step verification:

Allow namespace/user to build in Copr in general (=whitelist).
Allow PR author to build in that specific project (required write permission in that project).

Allow namespace/user to build in Copr in general (=whitelist).

Ah, from Copr perspective you (packit user) are the user who is allowed
to create projects and submit builds.  So the problem like who/what is able
to build in Copr through GH pull request is solely in your hands.
What I mean..., yes - there are some Copr rules which packit user needs to follow,
like legal POV and others, even packit docs say that.  But if someone crosses the border through PR, I'm sure it
would be logged somewhere and packit moderators can then administratively
remove such affected build, etc.

Allow namespace/user to build in Copr in general (=whitelist).

Ah, from Copr perspective you (packit user) are the user who is allowed
to create projects and submit builds.  So the problem like who/what is able
to build in Copr through GH pull request is solely in your hands.
What I mean..., yes - there are some Copr rules which packit user needs to follow,
like legal POV and others, even packit docs say that.  But if someone crosses the border through PR, I'm sure it
would be logged somewhere and packit moderators can then administratively
remove such affected build, etc.

Can we get rid of the whitelist as well then?

I'm reading this question once more, and now I'm sure it was not targeted to me :-) sorry for noise.

Can we get rid of the whitelist as well then?

I'm reading this question once more, and now I'm sure it was not targeted to me :-) sorry for noise.

Can we get rid of the whitelist as well then?

I'm reading this question once more, and now I'm sure it was not targeted to me :-) sorry for noise.

No worries, thanks for the great elaboration.

Can we get rid of the whitelist as well then? (I think you discussed this with @TomasTomecek some time ago.)

I'd say we can't get rid of it for good. Though we could create a better solution which would be more straightforward (e.g. after installing packit, we'd redirect to our page where we would provide "rules" of the service which our users would need to "accept").

Can we get rid of the whitelist as well then?

I'm reading this question once more, and now I'm sure it was not targeted to me :-) sorry for noise.

No worries, thanks for the great elaboration.

Can we get rid of the whitelist as well then? (I think you discussed this with @TomasTomecek some time ago.)

I'd say we can't get rid of it for good. Though we could create a better solution which would be more straightforward (e.g. after installing packit, we'd redirect to our page where we would provide "rules" of the service which our users would need to "accept").

To clarify what we want to do:

Do not require write permission for pull-request authors to trigger the build. (The task of this issue.)
Still require some validation of the project owners.

Adding to our board so we can start work soon.

To clarify what we want to do:

Do not require write permission for pull-request authors to trigger the build. (The task of this issue.)
Still require some validation of the project owners.

Adding to our board so we can start work soon.

I'd like to clarify the outcome of this card:

based on the title and description of the issue, we want to allow anyone creating PR against a repository to trigger the build (change included in the PR)
do we also want to apply similar set of rules to comments (e.g. /packit build)?
(change in PR allows only initial build after creating the PR, not comments by author of the PR without write permissions)
a. keep as is
b. allow author of the PR to trigger the build too (ignoring if author has or has not the write permissions to the repository)

I'd like to clarify the outcome of this card:

based on the title and description of the issue, we want to allow anyone creating PR against a repository to trigger the build (change included in the PR)
do we also want to apply similar set of rules to comments (e.g. /packit build)?
(change in PR allows only initial build after creating the PR, not comments by author of the PR without write permissions)
a. keep as is
b. allow author of the PR to trigger the build too (ignoring if author has or has not the write permissions to the repository)

I'd like to clarify the outcome of this card:

based on the title and description of the issue, we want to allow anyone creating PR against a repository to trigger the build (change included in the PR)

+1

do we also want to apply similar set of rules to comments (e.g. /packit build)?
(change in PR allows only initial build after creating the PR, not comments by author of the PR without write permissions)
a. keep as is
b. allow author of the PR to trigger the build by commenting too (ignoring if author has or has not the write permissions to the repository)

I'd say that we should allow PR authors to do /packit build: on the other hand, if some random Joe Foo Bar comments on a PR where he's not the author, we should not change commit statuses and probably comment in that PR that the user doesn't have permissions to do that.

I'd like to clarify the outcome of this card:

based on the title and description of the issue, we want to allow anyone creating PR against a repository to trigger the build (change included in the PR)

+1

do we also want to apply similar set of rules to comments (e.g. /packit build)?
(change in PR allows only initial build after creating the PR, not comments by author of the PR without write permissions)
a. keep as is
b. allow author of the PR to trigger the build by commenting too (ignoring if author has or has not the write permissions to the repository)

I'd say that we should allow PR authors to do /packit build: on the other hand, if some random Joe Foo Bar comments on a PR where he's not the author, we should not change commit statuses and probably comment in that PR that the user doesn't have permissions to do that.


feat(container-pull-script): store result from copy command

When integrating this script into an automated process, this output can be very useful, for example to feed the result into helm chart paramters automatically.

@elruwen thanks for opening up the PR. At this time, I don't think we want to add this specific functionality (although you are free to fork this project and make it work for you).
With that being said, I think you are correct in that we should provide a proper stdout at the end of any copy operation to let users know the path the image was copied to... something like:
image copied as: http://quay.io/test/falcon-sensor:7.19.0-5806.container.x86_64.Release.US-2

Ideally, you would be using ArgoCD or FluxCD (better helm support) in conjunction with a GitOps process. If a GitOps tool can't be used for some reason, you should use a custom config.yaml file that is passed to helm that contains all of your desired configuration vs saying setting everything via command line flags. From there, you could use --list-tags to get the tags if so desired. Of course at that point, it would be better to use this script to just grab your pulltoken and use skopeo directly to copy everything over as well as using skopeo to get your tags.

Even with gitops, I need the version number so I can write it to my argocd repo.
And then I need to parse the output of --list-tags. When I call the endpoint right now, the latest version is the last one in the list. But I don't see that behaviour somewhere documented. For a while this same endpoint was also listing sha256 sums at the end of this command, so if I would have blindly picked the last one I would have gotten something I don't want. That seems to be fixed now.
All doable, but imho unnecessarily complicated

And then I need to parse the output of --list-tags. When I call the endpoint right now, the latest version is the last one in the list. But I don't see that behaviour somewhere documented.

Yes - the last one is the latest version. When you say it's not documented, are you saying that you want to see something that says:
--list-tags                                    List all tags available for the selected sensor type and platform, sorted in ascending order

If so I think that is a valid ask. For context, we do the sorting here because the API's do not provide us with properly sorted output - and most tools like jq or even skopeo will not give you the correct sorted due to the fact that some of our containers are not in proper semver format.
So just thinking here - currently we offer the --get-image-path which gives you:

registry/repository:latest-tag

ex: registry.crowdstrike.com/falcon-container/us-2/release/falcon-sensor:7.19.0-5806.container.x86_64.Release.US-2
For me, this pretty much offers you all the information you would need to construct your copy-path in an automated fashion. I could see maybe just doing:
--get-copy-path to display the path where the image will get copied to, but at that point, my initial stdout of Image copied as: ... would be easy enough to get the same information.

We will not be merging this request. All the information is already there to build a copy-path if need be. As an example:
basename $(falcon-container-sensor-pull.sh -t falcon-container --get-image-path)

Will return your repository:latest-tag
And if you are already copying with the -c, --copy arg, then you already have your destination built-out.


Add BlockIdManagerSelector

Description
Adds the BlockIdManagerSelector to the list of available manager selectors. This selector returns a sorted list of managers by their block id, from greatest (newest) to least (oldest). Will be used to more effectively distribute tasks. Eventually, this selector will be used in conjunction with some method of sorting the task queue based on runtime to a) ensure tasks won't get assigned to managers that have a shorter remaining wall time than that of the task's execution time and b) put tasks with similar execution times on the same block, thereby resulting in a higher likelihood that the block completely empties of tasks, allowing the scale down logic to kick in.
Changed Behavior
Users can now choose the BlockIdManagerSelector() in the HTEX config.
Fixes
Work as part of #3323
Type of change

New feature

you might elaborate on "Will be used to more effectively distribute tasks."

switching this to draft status because @matthewc2003 is working on gathering evidence on when this does and does not help.

As a result of some experimentation I've found the following:

BlockID manager selector helps with workloads that see a varying amount of tasks over time. Example: Cholesky factorization workload run through the TaPS benchmarking suite
Random Manager Selector:

Block Id Manager Selector:

We see new blocks are prioritized with the blockID manager selector, when used with 'htex_auto_scaling', results in compute cost savings.
Doesn't really work with bag-of-tasks workloads. When all the tasks are put into the queue upfront, all blocks operate at near full utilization for the majority of the workload, which task goes where doesn't really matter.

some documentation ideas:
the same summary you posted above, put into a docstring for the block id manager selector, and then add to docs/reference.rst (perhaps in a new section)
add a description of manager_selector to the HighThroughputExecutor selector, with a link to the docstring on block id manager selector.

It would be good to get some testing in here for the newly introduced code.
There are a few things you could do:
make a test that invokces BlockIdManagerSelector.sort_managers with some example inputs and check that it sorts them the right way
launch a DFK with the BlockIdManagerSelector configured and check that a simple no-op task runs.
level 2 of that: run a configuration with 2 blocks, no scaling, run a few tasks and check that they all end up in the 2nd block and never in the first block: you should see that in the PARSL_WORKER_BLOCK_ID environment variable inside a task
@khk-globus might have some other ideas too around this space

I removed most of your PR description from the commit message, rather than harrassing @matthewc2003 to go rewrite it - it was mostly about stuff the isn't implemented, rather than stuff that this PR implements.


azidentity multitenant authentication API

Usage looks like this:
// cred will request tokens from the configured tenant unless another is specified in GetToken args
cred, err := azidentity.NewClientSecretCredential("primary-tenant", "client-id", "secret", &azidentity.ClientSecretCredentialOptions{
	// cred will (attempt to) authenticate in any requested tenant; can also list specific tenants here
	AdditionallyAllowedTenants: []string{"*"},
})

GetToken() returns an error when its arguments specify a tenant the credential isn't configured to allow.

I changed base to main because azidentity betas are shipping from there and converted it to a draft pending the azcore beta release.

/azp run go - azidentity - ci


Some bugs i´ve found on W10 Pro x64 20H1 update

Describe the bug

When i open Files, and navigate trough my folders and files, he suddenly crash. This happens after the update through Microsoft Store from 1.1 to 1.2 version. But when i uninstall the update, and reinstall it, the software runs very well.
When i select the black theme and close Files and open it again, it shows the white theme.
The software stay every time at the top, maybe an option to send it back. It happens with Illustrator projects for example, I open it and llustrator stay in background,

Expected behavior

Not to crash will be amazing
Select the black theme and stay :)
An option to send Files to background when open files like Illustrator projects

Screenshots
If applicable, add screenshots to help explain your problem.
Desktop (please complete the following information):

OS Version: Windows 10 Pro 20H1 v2004 compilation: SO 19041.508
App version: 1.2

Update to 1.3 and the first two issues will be gone 👍

thanxs, ive updated to 1.3 and actually works fine 👍


can i use in other android devices..?

It will probably work on other Samsung Android devices but I am not sure as I haven't got the time to update this library, and it is outdated now.

@Jaymin-Bhadani can you in other android devices, you could find it out in:
http://developer.samsung.com/s-pen-sdk/technical-docs/S-Pen-SDK-2-3-Tutorial

I have used this library it works fine upto Android O.S version 6.0 but not on above versions. Can any one tell me some solution to run it on Above versions


Update eks distro example with receiver to scrape k8s api-server metrics

This PR adds a new receiver to the eks distribution example to show users how they can make use of the prometheus receiver to scrape and report metrics from k8s api-server in a managed cluster like EKS.

I believe we should set it up by default instead of https://github.com/signalfx/splunk-otel-collector-chart/blob/main/helm-charts/splunk-otel-collector/templates/config/_otel-agent.tpl#L159

We can add it to be set as default for gke and eks distros. Haven't tested in AKS yet, but they probably expose these too. We'll have to decide on the default metrics to collect though cause there are a lot of metrics exposed.

Yeah I think we need to refactor how we collect control plane metrics.

Our default Splunk dashboard content requires a "metric_source" attribute like "metric_source: kubernetes-apiserver" for control plane metrics, it may be time to change this internally.
This last year, cloud hosted Kubernetes clusters added support for control plane metrics.
I'm not sure how well multi-zone control planes metrics are collected when using a service to reach a metrics endpoint, I think this was a potential issue before that may have been resolved. If this isn't an issue I would prefer to use services instead of direct host ports (_otel-agent.tpl#L159).


Why are almost all options greyed out? Rebound 11

Because we haven't made apps for everything yet 🙂


fix watching with a specified resource version

The watch code reset the version to the last found in the
response.
When you first list existing objects and then start watching from that
resource version the existing versions are older than the version you
wanted and the watch starts from the wrong version after the first
restart.
This leads to for example already deleted objects ending in the stream
again.
Fix this by not resetting to an older version than the one specified in
the watch.
It does not handle overflows of the resource version but they are 64 bit
integers so they should not realistically overflow even in the most loaded
clusters.
Closes https://github.com/kubernetes-client/python/issues/700

/assign @yliaog

Any update @yliaog ? Thanks!

This PR won't stop the 410 error, right? It will just stop the out of order processing?

the 410 resource version to old error is not handled by this.
This still needs to be handled by the application e.g. by restarting the watch at the last known resource version (which this PR does fix).


Creation of PostProcessingVariable

Is there already a possibility to create postprocessing variables?

Hello
yes.
hfss.variable_manager.set_variable("myvar", "1mm", postprocessing=True)
or  by giving a name with prefix post_
hfss["post_myvar"] = "1mm"


Updating versions to support rails 5.

Coverage increased (+0.2%) to 100.0% when pulling 48432f73c0d4303001837334010624903b6fef6e on feature/update-deps into 01d52786d7dc1ee0e1293f49ea3905e7eaa70bde on master.

Coverage decreased (-0.004%) to 99.747% when pulling 48432f73c0d4303001837334010624903b6fef6e on feature/update-deps into 01d52786d7dc1ee0e1293f49ea3905e7eaa70bde on master.

Coverage decreased (-4.4%) to 95.385% when pulling 8b4c802c51687a9db9fe393641c90c457454da06 on feature/update-deps into 01d52786d7dc1ee0e1293f49ea3905e7eaa70bde on master.

Coverage decreased (-4.4%) to 95.385% when pulling 8b4c802c51687a9db9fe393641c90c457454da06 on feature/update-deps into 01d52786d7dc1ee0e1293f49ea3905e7eaa70bde on master.

Coverage decreased (-4.4%) to 95.385% when pulling 8b4c802c51687a9db9fe393641c90c457454da06 on feature/update-deps into 01d52786d7dc1ee0e1293f49ea3905e7eaa70bde on master.

Coverage increased (+0.2%) to 100.0% when pulling 5a4f4a1a3f41696fe9506f844c0264030d968752 on feature/update-deps into 01d52786d7dc1ee0e1293f49ea3905e7eaa70bde on master.


Issues with Create Button

Describe the bug
User Id not handled in the API
To Reproduce
Steps to reproduce the behavior:
REQUEST
curl 'http://localhost:8080/orders'
-H 'Accept: /'
-H 'Accept-Language: en-GB,en-US;q=0.9,en;q=0.8'
-H 'Connection: keep-alive'
-H 'Content-Type: application/json'
-H 'Origin: http://localhost:8080'
-H 'Referer: http://localhost:8080/'
-H 'Sec-Fetch-Dest: empty'
-H 'Sec-Fetch-Mode: cors'
-H 'Sec-Fetch-Site: same-origin'
-H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/<IP_ADDRESS> Safari/537.36'
-H 'X-Requested-With: XMLHttpRequest'
-H 'sec-ch-ua: "Google Chrome";v="119", "Chromium";v="119", "Not?A_Brand";v="24"'
-H 'sec-ch-ua-mobile: ?0'
-H 'sec-ch-ua-platform: "macOS"'
--data-raw '{"name":"AbC test","create_time":"2023-12-01T22:17:11Z","address":"Manhattan","cost_amount":200,"status":"NEW","user_id”:25,”items":[]}'
--compressed
RESPONSE RECEIVED
ERROR CODE 500
Expected behavior
Create button should create a new order
Observed behavior
Error code 500

https://github.com/CSCI-GA-2820-FA23-003/orders/pull/101


Working through the coloring next


[User story] As a developer I want to return SCI scores so that I can calculate managed services

As a developer I want to return SCI scores so that I can calculate managed services
As discussed in this issue: https://github.com/Green-Software-Foundation/sci/discussions/235
For some things Carbon QL just needs to return an SCI score, e.g. for networking we don't want carbon ql to return separate E, I and M values, it's redundant, we just need to return an SCI score like Carbon / GB which you can then use to calculate your applications SCI score.
Networking is a managed service, you don't by the compute for networking you pay by some other param like GB.
Databases are another managed service.
Serverless are another managed service.
NOTE: To model this we can use some form of $ -> Carbon Emission Factor.

@jawache One of  the idea for converting $ to Carbon emission factor is here https://www.cloudcarbonfootprint.org/docs/methodology#cloud-usage-and-cost-data-source


Make the use of revquote.sty optional in ansrev.sty

Currently we give an error if main.quo is missing but we should only try to read this file if at least one \revquotefrom is in the document.

Fixed by 3e21b4df45561b907ebdd690e373bb2014bfaa24


Review payment & payout request schemas

Probably some test are failing by the API, must we skip them?


NIFI 7542 Upgrade additional jackson-databind versions.

Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
Description of PR
Enables X functionality; fixes bug NIFI-YYYY.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:

[x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?

[x] Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

[ ] Has your PR been rebased against the latest commit within the target branch (typically master)?

[ ] Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

[ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
[ ] Have you written or updated unit tests to verify your changes?
[x] Have you verified that the full build is successful on JDK 8?
[ ] Have you verified that the full build is successful on JDK 11?
[ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
[ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
[ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
[ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

[ ] Have you ensured that format looks appropriate for the output in which it is rendered?

Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

@mtien-apache LGTM. Do you have any remaining commits you need to make?

@MikeThomsen That was the last commit. Thanks.

Did a final pass with mvn dependency:tree | grep databind and it looks like you got everything.


null in query variables is wrongfully passed as a string "null"

I am using v1.25.0 of graphql-go-tools. I found out that a null value in the variables will be passed by graphql-go-tools as a string "null". It also happens in v1.23 too.
Using NewFederationEngineConfigFactory from 1.25.0, I am able to create a gateway
factory := graphql.NewFederationEngineConfigFactory

and using the NewGraphqlHTTPHandler in examples/federation/gateway/http to create a handler.
To showcase the problem, I put my mutation string and variables below
mutation ($city: String) {
  updatemember(id: "81", data: {city: $city}) {
    city
  }
}

{
	"city": null
}

However, at my graphql_datasource server, it recerives the following JSON. The city has a string value "null".
{"variables":{"a":"81","city":"null"},"query":"mutation($a: ID!, $city: String){updatemember(id: $a, data: {city: $city}){city}}"}

It only happens when null is assigned in variables. If null is in the input, it would be passed as null.
It's expected that the variables follow the same behavior as stated in graphql spec
Null Value

NullValue : null

Null values are represented as the keyword {null}.

Or did I use the tool in a wrong way? Any help is appreciated. Thanks in advance.

I think this should be solved by this PR, can you try?
https://github.com/TykTechnologies/graphql-go-tools/pull/145

I think this should be solved by this PR, can you try?
TykTechnologies#145

Thanks for the quick replay. I can gladly confirm that it does fix it.

Should now be fixed with 1.26.0.
Can you confirm (and close) or report additional problems.

Tested with 1.26.1 and it's fixed.


🐛 BUG: Cannot read properties of undefined (reading 'fetch')

Which Cloudflare product(s) does this pertain to?
Pages, Workers Runtime
What version(s) of the tool(s) are you using?
3.6.0
What version of Node are you using?
18.16.1
What operating system are you using?
WSL2
Describe the Bug
I'm trying to use SSR with cloudflare workers on a small-medium size site that is a blog with some extra marketing pages. I need to use SSR for the preview environment of the CMS.
How to reproduce:
Create wrangler toml file with the following:
name = "sis-ssr-worker"
main = "dist/_worker.js"
compatibility_date = "2023-08-14"

Follow the get started guide here with an existing Astro project
https://developers.cloudflare.com/workers/get-started/guide/
deploy the worker.
I get several
Please provide a link to a minimal reproduction
https://sis-ssr-worker.bianca-15d.workers.dev/
Please provide any relevant error logs
Log error. There's more in the logs, but it shares some sensitive information. Let me know if there's anything else I need to provide!
  "outcome": "exception",
  "scriptName": "sis-ssr-worker",
  "diagnosticsChannelEvents": [],
  "exceptions": [
    {
      "name": "TypeError",
      "message": "Cannot read properties of undefined (reading 'fetch')",
      "timestamp":<PHONE_NUMBER>946
    }
  ],

[mf:inf] GET /_astro/hoisted.2302b26c.js 500 Internal Server Error (267ms)
[mf:inf] GET /_astro/page.9f2ff80f.js 500 Internal Server Error (270ms)
[mf:inf] GET /_astro/hoisted.fe6862c6.css 500 Internal Server Error (270ms)
[mf:inf] GET /astro/index.8cb5bef9.css 500 Internal Server Error (273ms)
[mf:inf] GET /astro/...slug.c642a811.css 500 Internal Server Error (276ms)

I am receiving this as well.
It works when deployed but not when using wrangler dev (with custom builds)

@thejackshelton thanks for filing this issue, and sorry for the delayed response. I'm not sure if you're still having issues with your Astro site, but if you are we currently recommend using the create-cloudflare (C3) tool to start new projects since it will use Astro's scaffolding tool and then install Cloudflare-specific adapters and config. The docs you need are here: https://developers.cloudflare.com/pages/framework-guides/deploy-an-astro-site/

@thejackshelton  please let us know if using create-cloudflare (C3) per @admah 's recommendation above addresses the issue, or if you still run into the same issue when using C3. thanks!

hi @thejackshelton :) we haven't heard from you in a while, so we’re going to close this issue for now, but if this is still a problem, please feel free to open a new issue referencing this one. please be sure to include an updated summary and any additional requested details, followup, or context and we will revisit and investigate further. thanks!


Add framework clients to OMASs

Each Open Metadata Access Service (OMAS) Client should have two framework clients plus one or more specialized clients. This enhancement is to ensure that the two framework clients are added to all OMASs.
Description
As part of the changes client modules named ConnectedAssetClient and OpenMetadataStoreClient were added to the OMAS under access-services
Related Issue(s)
#7857
Testing
A clean build with all FVTS was done on the project
Release Notes & Documentation
NA
Additional notes
NA

The new clients, where I can see them look good 👍
However, there are 993 files changed in this PR. Most of these are just reorganizing the imports of files unconnected with the change. I think there must be a setting wrong in your IDE that is causing this. You need to work out what that is and resubmit with a PR that only includes the files that you changed (deliberately :) .

Hello @mandy-chessell ,
Thanks for pointing that out. I have made the changes and asked for the review again. Please have a look when you can.
Happy Holidays and a new year !!!


fix(doc): improve the blob command spec

This PR duplicates with #1118.

Looks like #1118 doesn't cover all of this PR?

As discussed with @Two-Hearts , PR #1118 will include this PR.


Pattern for Nginx Monitoring

Issue #, if available: #6
Description of changes: Add existing EKS scenario for nginx monitoring
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@joozero Please refer to this PR - https://github.com/aws-observability/cdk-aws-observability-accelerator/pull/76 for template to do nginx monitoring.

@joozero Is the PR ready for review. I see not all comments are fixed. Also i see GH actions which needs to be fixed.

/do-e2e-test single-new-eks-opensource-observability destroy


[new-rule] Add no-unnecessary-type-check rule

PR checklist

[x] Addresses an existing issue: fixes #4733
[x] New feature, bugfix, or enhancement

[x] Includes tests

[ ] Documentation update

Overview of change:
Adds the rule no-unnecessary-type-check (see https://github.com/palantir/tslint/issues/4733 for more details)
Is there anything you'd like reviewers to focus on?

Not sure how to add documentation - is there a command or is it manual?
yarn verify is failing because there are some places in this repo where this rule fails (see below for example). Should I try to fix all these cases or just add a disable-line for this rule where the errors occur?

/Users/zkirsch/Documents/tslint/src/configuration.ts
This comparison is unnecessary because the left side is never undefined (no-unnecessary-type-check)
  151 |     inputFilePath?: string,
  152 | ): string | undefined {
> 153 |     if (suppliedConfigFilePath != undefined) {
      |        ^
  154 |         if (!fs.existsSync(suppliedConfigFilePath)) {
  155 |             throw new FatalError(
  156 |                 `Could not find config file at: ${path.resolve(suppliedConfigFilePath)}`,

This is my first time working the TSLint or anything to do with the TS compiler so please let me know if I did anything incorrectly or there's a corner case I didn't consider.

CHANGELOG.md entry:
[new-rule] no-unnecessary-type-check

Thanks for your interest in palantir/tslint, @zachkirsch! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.


[AVM Module Issue]: avm/res/api-management/service - certain versions reaching end of life

Check for previous/existing GitHub issues

[X] I have checked for previous/existing GitHub issues

Issue Type?
Feature Request
Module Name
avm/res/api-management/service
(Optional) Module Version
No response
Description
Hi @tony-box,
As it's been communicated here, some versions of the Azure resource used in your module have been retired. Please see more details here: https://techcommunity.microsoft.com/t5/apps-on-azure-blog/post-retirement-action-for-app-service-environment-v1-v2-logic/ba-p/4233407
Please make sure that

your module is aligned with these changes (it might already be, but I didn't see the parameter value called out in the article above, so please check and confirm)
you have the following blurb added to the Notes section of your module's readme.md (Feel free to also include a pointer to the public announcement linked above.):
"Certain versions of this Azure resource reached their end of life. The latest version of this module only includes supported versions of the resource. All unsupported versions have been removed from the related parameters."
you have the following blurb added to the related parameter's description:
"Certain versions of this Azure resource reached their end of life. The latest version of this module only includes supported versions of the resource. All unsupported versions have been removed from this parameter."

Thank you!
(Optional) Correlation Id
No response

@matebarabas it appears that using a newer version of the APIM ARM API will default to deploying a stv2 APIM sku. There isn't an explicit parameter to define what APIM sku to deploy: https://learn.microsoft.com/en-us/azure/api-management/api-management-using-with-vnet?tabs=stv1#prerequisites
I have cross referenced a few other places and it seems that in the past, (see https://techcommunity.microsoft.com/t5/azure-paas-blog/compute-platform-versions-for-azure-api-management-service/ba-p/2836971) to get a stv2 SKU, you had to deploy APIM with a certain set of networking requirements. But the latest APIs will always deploy a v2 SKU. I can do a little more digging to validate this. It's not documented very well.

Thanks, @tony-box, for looking into this! When your research is concluded, please still consider adding the above blrub to the over Notes section of the readme.md - I understand though, that it seems there isn't any parameter to add the same blurb to, but I still believe it might be worth to clarify that this module only supports the latest version/tier/family/sku. Thanks!

Confirmed from engineering that the stv1 should no longer be deployable using bicep. I've added text to the notes section of the Readme.md file in #3342

Thanks for the fix/update, @tony-box!


Support for multiple types as parameters

Rightn now the looger methods only support receiving Strings but using console.log directly will allow us to use more types

Closed by #10