Simplify the tedge config crate and command

The tedge_config crate has aged badly as most of the requirements driving its design have been removed.

Adding a simple property to the tedge_config file is tedious. Several files have to be updated with specific trait implementations for each new configuration property. It's really easy to get the details wrong (notably with misleading help information).
Even accessing the config is tedious. You have to know that tedge_config.query(MqttClientHostSetting)? is to be used to get the value set with tedge config set mqtt.client.host and store in tedge.toml along mqtt.client_host.
Finally, extracting an MqttConfig from the tedge_config is done again and again by all the MQTT clients.

Expectations
I would expect:

A single TedgeConfig struct (a POJO !) that contains all the settings and can be used directly (as in tedge_config.mqtt.client.host) where the same path is used by the user, the toml file and the rust code.
Default values provided by an impl Default for TedgeConfig.
Some helpers implementation to extract frequently used sub-configs (as an impl From<TedgeConfig> for MqttConfig).
Compatibility with the tedge config command, to get, set, unset properties as well as to get some documentation.
Openness to future extensions as accessing parts of the config over MQTT.

Proposal
Using figment one can populate a regular TEdgeConfig struct from different sources: starting from default values, merged with the content of the tedge_config file, merged with values extracted from the environment and possibly merged with some computed keys (as the device.id).

The list of available properties as well as their documentation can be provided by a toml file included in the source code, with the expected structure but with doc comments instead of values. This will be used for tedge config list and  tedge config list --doc.
serde_ignored can be used if we need to keep the exact same behavior for  tedge config list as today (i.e. to emit only the properties that have been explicitly set).

Impacts

All the tedge executables are impacted (rg tedge_config.query | wc -l returns 68 locations).
Do we keep the public names of each property (as given to the tedge config command) or those of the internal structure of the tedge.toml file?
Do we have to provide an update-tedge-toml script?

Default values provided by an impl Default for TedgeConfig.

Very much agree, for me it seems like the current implementation just increases boilerplate without providing much in return.

Some helpers implementation to extract frequently used sub-configs (as an impl From<TedgeConfig> for MqttConfig).

Wouldn't just &tedge_config.mqtt suffice (if I'm correct in assuming that it's of type MqttConfig)? We're just extracting part of the config, and any code using the config should not modify it, unless it's tedge config set command, but it just modifies the config file, so config object is never mutated. So config object can be immutable and we can take any number of shared references to it and its parts.
Some additional thoughts:

Some config options implying other options: Example: using MQTT port 8883 implies security so we can know whether to use TLS transport by matching on the port, but a user could also host either secure or insecure MQTT broker on any port, so matching on port number alone would be insufficient. We could have a configuration setting to select transport type, but it would have different default values depending on the port setting. I guess most reasonable thing to try first would be to handle it in the client code, and only consider adding complexity to config if it's too tedious to do this way.
Flattening object key names for tedge config: The issue was that environment variables should not contain . characters and therefore we should map all . characters to _. However, some key names include _, so to separate a section we should use a double underscore (__)
Discussion:  #1783
However, I think it might be possible to avoid double underscores and handle ambiguities by using serde's various attributes in combination with figment's Env provider.
Feature toggles. They can be a way for users to opt-in to use experimental features, or be an alternative to long-lived feature branches for developers collaborating on particularly complex features. If we want to consider them, then arguably config files would be the best place for them.

@reubenmiller and @albinsuresh : what do you think about the user impacts / benefits of this proposal?
@albinsuresh , @jarhodes-sag , @Bravo555 : what do you think of feasibility of this proposal?

Yes I see definitely see value in this, as extending the configuration should be a very quick/easy/no-fuss thing. We will just have to discuss exactly what the user-impact will be in terms of renaming/normalizing properties etc, as that will guide whether it worth while adding an upgrade step.

However, I think it might be possible to avoid double underscores and handle ambiguities by using serde's various attributes in combination with figment's Env provider.

The difficulty here arises from the fact that we have to be able to derive the structure from the environment variable name without referring to what the target config type is. Whatever we try and do, the result of deserialising from the environment variables to TEdgeConfig will be the same as deserialising to toml::Value and then deserialising that to TEdgeConfig, because serde depends on the format to dictate the structure of the value. I've run into similar problems decoding hex-encoded IP addresses in CSV which are normally treated as either strings or integers but because 000000E5 parses as f64 in Rust.
I do agree that simplifying this is useful, and to that end I suggest we impose a limit on the structure of the config to remain just the current two levels deep. That would allow a relatively simple approach of splitting the key on the first _ (after the TEDGE_ prefix) and then assuming any further _ are part of the field name. If we desperately needed to add a third level to some configuration later down the line, we could support this as we can provide our own logic to interpret some environment variable names specially, but this would involve hard-coding the relevant support in.

To give a clear answer on what I think we should support in the future, I think preserving the existing toml format makes sense. It will likely be easier to add clear error messages or deprecation warnings for the existing paths to tedge config than to update tedge.toml (and detect in the future whether this has already been done). This also makes simplifying the environment variable names easier and less error prone. I'll open a separate improvement request for that topic as implementing this refactoring will be a significant undertaking in its own right and that aspect is decidedly tangential to the original goal of this issue.

I might have one more requirement for the new config, which is handling compound configuration options where all the fields have to be set for configuration to be consistent. One example is MQTT client authentication I'm currently adding, which requires two options to be set:

Path to the client certificate
Path to the client private key

Note that there are no reasonable default for either of these options, while for other options eg. hostname-port pairs there is usually a default value for the port which enables us to only pass the hostname.
Now, if we want to make configuration an object that can contain other objects, then I'd argue that it would make sense for the configuration object to contain a field like Option<MqttClientAuthConfig>, which would be guaranteed to contain both certificate and the key, so that the command code won't have to bother about handling cases when only one of them is present.
The issue appears however, when we use tedge config command to edit the configuration. tedge config only works on single keys, so if a user only sets one option, it will leave configuration in an incomplete state. I see two potential solutions:

For some options, require the user to set other options for the configuration to become valid. So e.g. when setting mqtt.client.auth.cert_file there could be a hint to also set mqtt.client.auth.key_file. When the second key is not set, any tedge command will fail because configuration could not be parsed successfully, effectively requiring user to set the other required option. I think it's the easiest and most effective option.

Change tedge config set command to be more TOML-aware, so that it can set compound options. It could look something like this: `tedge config set mqtt.client.auth cert_file=/path/to/cert key_file=/path/to/key

Don't support compound options, effectively leaving all the keys optional as they are now. This moves the code handling presence/absence of different options into the code using the configuration, again, as it is now. I'd recommend against it, as by supporting compound options, we could remove some complexity from  command code, as well as remove all the empty tables that are present in tedge.toml file, which we need to have because instead of top-level fields being optional, it's all the low-level fields:
[device]

[c8y]
root_cert_path = '/etc/ssl/certs'

[az]

[aws]

[mqtt]
client_host = 'ubuntu-20'
client_port = 8883
client_ca_file = '/etc/mosquitto/ca_certificates/'

[http]

[software]

[tmp]

[logs]

[run]

[firmware]

[service]

pub(crate) struct TEdgeConfigDto {
    /// Captures the device specific configurations
    #[serde(default)]
    pub(crate) device: DeviceConfigDto,

    /// Captures the configurations required to connect to Cumulocity
    #[serde(default)]
    pub(crate) c8y: CumulocityConfigDto,

    #[serde(default, alias = "azure")] // for version 0.1.0 compatibility
    pub(crate) az: AzureConfigDto,

    #[serde(default)]
    pub(crate) aws: AwsConfigDto,

    #[serde(default)]
    pub(crate) mqtt: MqttConfigDto,

    #[serde(default)]
    pub(crate) http: HttpConfigDto,

    #[serde(default)]
    pub(crate) software: SoftwareConfigDto,

    #[serde(default)]
    pub(crate) tmp: PathConfigDto,

    #[serde(default)]
    pub(crate) logs: PathConfigDto,

    #[serde(default)]
    pub(crate) run: PathConfigDto,

    #[serde(default)]
    pub(crate) firmware: FirmwareConfigDto,

    #[serde(default)]
    pub(crate) service: ServiceTypeConfigDto,
}

require the user to set other options for the configuration to become valid. So e.g. when setting mqtt.client.auth.cert_file there could be a hint to also set mqtt.client.auth.key_file. When the second key is not set, any tedge command will fail because configuration could not be parsed successfully, effectively requiring user to set the other required option. I think it's the easiest and most effective option.

I realized that it will be trickier that it sounds, because of how tedge config set currently works, which is AFAIK:

Read and parse the entire TOML file into our configuration object
Modify the relevant field in the relevant configuration object
Serialize back to TOML and save in the filesystem

Which means that currently it's impossible for tedge config set to leave configuration in an incomplete state, so if we were to have the following configuration object:
pub(crate) struct MqttClientAuthConfig {
    pub cert_file: FilePath,
    pub key_file: FilePath,
}

We couldn't set only one of the fields at a time.
The root of the problem seems to be, for updating configuration we want to make every field effectively optional, but for reading we want to enforce some tables to define all required keys.
Possible solutions:

When setting fields, don't deserialize, and instead work on loosely typed TOML representation. There would have to be some mechanism preventing us from setting fields which don't exist in our config objects.
Some serde attributes/macro magic to transform our config objects that makes all the fields optional, and then serializing to and deserializing from that. In practice means we use different models for serialization and deserialization. Can become really ugly because 90% of the config doesn't need it.
Just give up and have all the fields be optional.

I realized that it will be trickier that it sounds, because of how tedge config set currently works, which is AFAIK:
1. Read and parse the entire TOML file into our configuration object

2. Modify the relevant field in the relevant configuration object

3. Serialize back to TOML and save in the filesystem

Which means that currently it's impossible for tedge config set to leave configuration in an incomplete state, so if we were to have the following configuration object:
pub(crate) struct MqttClientAuthConfig {
    pub cert_file: FilePath,
    pub key_file: FilePath,
}

We couldn't set only one of the fields at a time.

In #1916, I haven't explicitly dealt with this problem, but I think I have a reasonable solution. tedge config is essentially using a separate interface to everything else to read individual keys, and each key has an (auto generated) getter method in NewTEdgeConfig. We can, if we want to, not generate a getter for a given DTO field and instead implement one manually without affecting tedge config.
Once we've done this, we can manually create a getter fn mqtt_client_auth(&NewTEdgeConfig) -> Result<MqttClientAuthConfig, ConfigSettingError> which could generate the new struct on the fly, and warn the user appropriately if they haven't set both keys (or it could return Result<Option<MqttClientAuthConfig>, ...>, which would be an error if only one field is set, Ok(None) if neither is  set, and Ok(Some(...)) if both are set).
There might be a little bit of attention required to get the user flow for this working nicely. I think keeping tedge config set simple like it is currently is nice, but we probably want to tell the user to go and configure the other setting (if they haven't already).
@Bravo555 does that sound like a reasonable compromise to you?

@jarhodes314
Currently reviewing the PR. It may take some time, so for now I'll be addressing just the comment. If I have any more questions or comments after having reviewed the PR, I will add them here in the next comment.

In #1916, I haven't explicitly dealt with this problem, but I think I have a reasonable solution. tedge config is essentially using a separate interface to everything else to read individual keys

Okay, so with tedge config using a different interface to read individual keys, it sounds a bit like the option 1 I proposed in earlier comment: tedge config not fully deserializing, but working on a loosely typed TOML.

and each key has an (auto generated) getter method in NewTEdgeConfig. We can, if we want to, not generate a getter for a given DTO field and instead implement one manually without affecting tedge config.
Once we've done this, we can manually create a getter fn mqtt_client_auth(&NewTEdgeConfig) -> Result<MqttClientAuthConfig, ConfigSettingError> which could generate the new struct on the fly, and warn the user appropriately if they haven't set both keys (or it could return Result<Option<MqttClientAuthConfig>, ...>, which would be an error if only one field is set, Ok(None) if neither is set, and Ok(Some(...)) if both are set).

Okay, so we can override these automatically generated getters in order to run some verification logic that creates some new configuration struct.
But I think it creates some problems:

The getter function can fail: this forces the calling code to handle the error case of invalid configuration state. But the calling code can't meaningfully recover from this error, and has to return the error as well, which just results in a crash. It will be the same in all the places which work with these compound config objects. The way to relieve client code of this complexity is to crash as soon as the config is first read and deserialised. This is good for us, because once we've deserialised, we know that configuration is valid and consistent, and are able to guarantee this to the config clients. But I'd argue this is good for the user as well - reporting invalid configuration as soon as possible makes them fix the configuration as soon as possible, instead of only when using the feature to which an option is related. One can argue that making this a warning would provide the same value for the user, but I'm still for making this a hard error.
Can't move out of NewTEdgeConfig: When we have a function signature like &NewTEdgeConfig -> MqttClientAuthConfig, it implies that we can call it multiple times, creating multiple MqttClientAuthConfigs from a single &NewTEdgeConfig, which in turn, realistically, implies cloning. I agree that perhaps I'm engaging in a bit of premature optimisation here, but I think performance goes hand-in-hand with simplicity; I think ideally we'd want to have a single immutable TEdgeConfig object, fields of which we can easily borrow. If this is impractical, or I'm misunderstanding how we use the config, I'm open to having my mind changed. It doesn't affect the client code as much as 1, so it's not as important.
Related to 2, the idea of having field getters themselves. What I think would be more idiomatic in Rust and result in less code and complexity, would be to have a single getter for configuration, NewTEdgeConfig::get() -> &NewTEdgeConfig, which would be the only point of access to the configuration object. It would guarantee immutability and we could access parts of it by just borrowing the fields. But here I also may be missing or misunderstanding something, if so, please do correct me.

I'll add that there were some changes in new versions of toml crate, which I think are relevant to us. toml was changed to use toml-edit under the hood, which AFAIK instead of deserializing, updating, and serializing, updates the TOML file directly. It's what is used in cargo add plugin. We could potentially use it to simplify tedge config and enable partial updates.
@didier-wenzek what's your opinion?

The getters also exist to provide default values. I guess we could do a conversion from TEdgeConfigDto -> something like it but with None replaced by T::default() where relevant, and this might simplify things. We could do the conversion to MqttClientAuthConfig there and provide the relevant warnings/errors then. I'm still not sure exactly how I'd want that to work for fields without defaults. Option seems sensible but will almost invariably lead to inconsistent error messages when the value is required but not set, which is why the getters use a Result<_, ConfigSettingError> instead.

My general opinion re cloning was that TEdgeConfigDto is tiny and borrowing the relevant fields was going to be a pain, particularly to offer support for borrowing String fields as &str rather than &String. This is one of the problems that goes away.

I'm not convinced I want to lean heavily on toml_edit as it makes things a lot more error prone than updating a DTO, although I will admit I last thought about this before I added support for typed updates via the ConfigurationUpdate enum. Verifying a proposed change can be deserialized to ConfigurationUpdate before updating the config to include it should be sufficient, and this allows us to get the correct toml representation.
The real issues with toml_edit come from aliases. If someone has the following config:
[azure]
url = "something.azure.net"

and we trivially updated az.url, we would get:
[azure]
url = "something.azure.net"

[az]
url = "new.example.com"

which, regardless of how serde deals with it, is a bad thing to have.
One related thing that's nice (IMO) with the way we currently do things is the transparent updating of aliases we get by overwriting the entire configuration. We can rename any field and the next time someone writes to the config via the tedge CLI, any outdated field names are replaced (though this isn't a big issue, we don't tend to rename fields in the toml).

2. My general opinion re cloning was that TEdgeConfigDto is tiny

rust-analyzer tells me it's 744 bytes, at least on my machine. So not that big if we clone it infrequently, but not exactly tiny either.

and borrowing the relevant fields was going to be a pain, particularly to offer support for borrowing String fields as &str rather than &String. This is one of the problems that goes away with your proposed approach of lending an entire struct.

I might be misunderstanding, but what would be the problem here? String derefs to str, so when somebody borrows &String they can just call str methods on &String or use .as_str() to get &str, right?

2. My general opinion re cloning was that TEdgeConfigDto is tiny

rust-analyzer tells me it's 744 bytes, at least on my machine. So not that big if we clone it infrequently, but not exactly tiny either.

and borrowing the relevant fields was going to be a pain, particularly to offer support for borrowing String fields as &str rather than &String. This is one of the problems that goes away with your proposed approach of lending an entire struct.

I might be misunderstanding, but what would be the problem here? String derefs to str, so when somebody borrows &String they can just call str methods on &String or use .as_str() to get &str, right?

You're right about it not being trivially small. I really meant cloning fields of tedge config, where we usually don't read many. There were also some lifetime issues with macros, and the fact a lot of the config implements copy so people would have to likely start manually dereferencing everywhere.
All of these are problems that are solvable with the existing solution, but it is not a solution I wanted to invest significant effort into until I had some agreement on whether the getters worked well at all, and I'm fairly convinced now that they aren't making anything better than the alternative solution I proposed, where we convert the DTO into a PORS with the relevant defaults filled in.

Closing as this work will be finalized with https://github.com/thin-edge/thin-edge.io/issues/2019


Unable to use remote schema

Describe the bug

When trying to use graphql-eslint with a remote schema via .graphqlrc.yml (async) schema, eslint throws at @graphql-eslint/require-id-when-available.
error [graphql-eslint] Error while loading schema
[Error - 2:57:07 PM] ESLint stack trace:
[Error - 2:57:07 PM] Error: Error while loading rule '@graphql-eslint/require-id-when-available': Rule 'require-id-when-available' requires 'parserOptions.schema' to be set and schema to be loaded. See http://bit.ly/graphql-eslint-schema for more info

As a matter of fact, I know that my rc works just fine because I am also using graphql-codegen to generate my code with it, so the loading of the schema is not a problem, though, it does require authentication.
schema:
  - "https://…/graphql":
      headers:
        X-Auth: "…"
documents: "src/graphql/**/*.{graphql,gql}"

When using a local copy of the schema (sync), it works just fine:
schema: schema.gql
documents: "src/graphql/**/*.{graphql,gql}"

To Reproduce
Steps to reproduce the behavior:
My config looks like this:
.eslintrc
{
  "root": true,
  "overrides": [
    {
      "files": ["src/**/*.{graphql,gql}"],
      "extends": ["plugin:@graphql-eslint/operations-recommended"]
    }
  ]
}

You can reproduce it with a public schema like SWAPI
schema: "https://swapi-graphql.netlify.app/.netlify/functions/index"
Expected behavior

ESLint to not throw and work with a remote schema

Just to give a quick update, I see that it's failing here:
https://github.com/dotansimha/graphql-eslint/blob/58a3991162e8863567923ef9c7e0c2526dcd0ad3/packages/plugin/src/schema.ts#L24
it's failing because it's trying to load the schema synchronously, which obviously will fail for a remote schema.

Can be closed... there was something weird with my editor. It is now working as intended 👍


Add env to configure the RSA key size of workload certificates

This PR adds a new env, "WORKLOAD_RSA_KEY_SIZE" to make the RSA key size of workload certificates configurable. Note that there is already a similar env for configuring the RSA key size of self-signed CA. Both of these default to 2048 bits. Users who prefer to have a higher key size for better security can benefit from this env.

Hi @sha-rath. Thanks for your PR.
I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

/ok-to-test

@sha-rath: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name
Commit
Details
Required
Rerun command

release-notes_istio
d0807461d0604ba1a29dc209e7ea1d04346256d9
link
true
/test release-notes_istio

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@sha-rath: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name
Commit
Details
Required
Rerun command

release-notes_istio
d0807461d0604ba1a29dc209e7ea1d04346256d9
link
true
/test release-notes_istio

unit-tests_istio
d0807461d0604ba1a29dc209e7ea1d04346256d9
link
true
/test unit-tests_istio

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

/test unit-tests_istio

@sha-rath: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name
Commit
Details
Required
Rerun command

integ-pilot-multicluster_istio
8ff121715cd428eff01e59303bd381a143d3cf37
link
true
/test integ-pilot-multicluster_istio

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

/test integ-pilot-multicluster_istio

@sha-rath: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name
Commit
Details
Required
Rerun command

integ-pilot-multicluster_istio
8ff121715cd428eff01e59303bd381a143d3cf37
link
true
/test integ-pilot-multicluster_istio

Instructions for interacting with me using PR comments are available here.  If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

One confusion: If 2048 is not secure enough, why not change to 4096_

@hzxuzhonghu It apparently impacts provisioning of many certs on large clusters. Also, there are some tls handshake concerns for latency and cpu time. Hence, the default was decided to be kept as 2048 bits. However, in my deployment, I have set the keysize to 3072 based on a security requirement we have.

/retest

@myidpt could you please review/approve this PR?

@howardjohn can you pls take a look into this?

cc @ericvn

@ramaraochavali would you like to review this PR?

Okay...thanks for the quick review!

@jacob-delgado @lizan could you pls help in getting this merged?

cc @howardjohn

@istio/wg-networking-maintainers PTAL

Not stale

@myidpt can you take a look?

Adding @lizan @liminw for review..


Ingredient quantity's decimal place flickering

Issue Description
I noticed that the ingredient quantities briefly show their decimal places before reverting to whole numbers, especially when changing the servings. This behavior causes a noticeable flicker in the UI.
Steps to Reproduce

Open a recipe with ingredient quantities that include decimal places.
Change the number of servings.
Observe the quantities momentarily displaying decimals before switching to whole numbers.

Root Cause
It appears that the HTML sent to the browser initially includes the decimals. However, a JavaScript script then modifies these values to whole numbers, causing the flickering effect.
Proposed Solution
To prevent this flickering, I propose that we handle the formatting on the server side using minijinja. We can remove the decimal places for whole numbers before sending the HTML to the client and thus eliminate the need to format the numbers client-side.
If this solution sounds good, I'd be happy to open a PR to implement this change.

Hello! This was a decision during development.
The reason to format numbers in the browser is to take advantage of the browser Intl API because it's a complex topic. It's not just the decimal place, depending on the language and how big or small the number is, the format changes. The same approach is also used for formatting times.
With your proposed solution it will still flicker in some languages for some numbers, but it will be better as whole numbers will not. Feel free to open a PR. Same thing for #29 as both are editing templates. 😄


feat: ECR repos check script ignore repos whose names contain "test".

The type of this PR is: Feat
This PR solves PHIRE-210
Description

Ignore repos whose names contain "test".

Un-related re-factoring:

Delete dummy src/ecr_check_repos_for_terraform/test.
Delete src/ecr_check_repos_for_terraform/ecr_check_repos_for_terraform/config.py. Move setup code to main script. config.py was introduced to make it easier for modules to access app-wide configs. This was useful because originally scripts had many functions. So for example, src/scriptX would have multiple files each with multiple functions each having to access an app-wide config. Without config.py, the functions had to pass the configs around. But this is no longer the case. Currently most functions have been migrated to src/lib directory, so most scripts have only a few functions, making the advantage of config.py moot.

Are there many test repos? Maybe it's fine to alert about them, because some action is required to clean them up eventually.

Are there many test repos? Maybe it's fine to alert about them, because some action is required to clean them up eventually.

I see only 3. Yeah, a clean up job might be good, as a separate job.


workspace.getActiveTextEditor() etc don't work if the text editor is not a PaneItem

The docs for workspace.getTextEditors() suggests that all TextEditors anywhere will be returned, just as workspace.getActiveTextEditor() should seemingly return the currently active TextEditor no matter what.
This is obviously at odds with the implementation, which only considers TextEditors which are registered as PaneItems. In the ink package, we have a Console-class, which extends PaneItem and can contain TextEditors as input cells (those are properly registered, I think), but one can't access them via one of the commands above (also see #75 for why this is problematic).
So either the documentation should clarify that only certain TextEditors will be considered or the implementation should be changed to searching the TextEditorRegistry or something like that.
I can have a look at implementing this if you think that'd be a proper solution.

@nathansobo do you have thoughts on whether this is an api or documentation bug? I lean toward documentation.

Yeah it's a documentation issue. The atom.workspace global really implies that we're talking about text editor pane items. For other editors, we could potentially add a method to the atom.textEditors global which is intended to deal with all editors. An approach that would work today would be to get the focused element via document.activeElement, but when the shadow DOM goes away the result will be the hidden input element embedded within the editor and won't be super convenient to resolve to an actual editor, hence my suggestion that we might add an API.

Yeah, I was kinda unsure what workspace actually implied. Good to have that cleared up! Still don't quite understand the need for getting editor pane items as opposed to editors, but then again I haven't put much thought into this yet.
I think adding some a getActiveTextEditor method to atom.textEditors would be great, but I don't think we have any real need for that yet -- it's just surprising that some method of workspace (which is supposed to "Represent the state of the user interface for the entire window.") only works for certain parts of it.
Maybe it would make sense to deprecate atom.workspace.getActiveTextEditor etc in favor of that new API? I'd guess there are some packages out there which should probably work with text editors everywhere but don't right now (and I don't think to many packages contain non-PaneItem text editors anyways).
Sorry if I'm rambling (was never any good at designing APIs and stuff) and thanks for the feedback!

@pfitzseb Yeah, the workspace actually has a paneContainer, but it's not part of the public API. In retrospect it could probably be more clear. Workspace is the object that handles panes and panels, and since we never have a panel containing just an editor it felt natural to just have these methods imply pane items. At this point, this is probably too central of an API in the package ecosystem for this to be worth changing no matter what.

Yeah, deprecating/changing that is probably more trouble than it's worth.
I'll try to figure out some way to improve the docs and maybe have a look at how to implement atom.textEditors.getActiveTextEditor or something similar.


Prevent escalation of privileges

Backport of #845

CI got stuck. Merged manually to 12-0-stable.


fix(*): 🐛 strict matches should appear first

Codecov Report

Merging #151 (83a3745) into master (ebfacea) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
+ Coverage   96.08%   96.15%   +0.07%
==========================================
  Files          31       31
  Lines         383      390       +7
  Branches       62       63       +1
==========================================
+ Hits          368      375       +7
  Misses         10       10
  Partials        5        5

Impacted Files
Coverage Δ

src/prompts/scope.ts
100.00% <100.00%> (ø)

src/prompts/type.ts
100.00% <100.00%> (ø)

src/utils/fuzzy-search.ts
100.00% <100.00%> (ø)

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 ebfacea...83a3745. Read the comment docs.


dua (non interactive) lost its ability to process files

Hi @Byron :wave:
It seems that starting with version 2.21.0 dua lost its ability to process single files in non interactive mode.
With 2.20.3:
~
tim@fs ❯ dua -V
dua 2.20.3                                                                                3ms
~
tim@fs ❯ dua emoji-test.txt
 628.00 KiB emoji-test.txt                                                                                3ms
~
tim@fs ❯

With 2.21.0:
~
tim@fs ❯ dua -V
dua 2.21.0                                                                                3ms
~
tim@fs ❯ dua emoji-test.txt
Error: Not a directory (os error 20)                                                                                3ms
~
tim@fs ❯

The same applies to 2.22.0:
~
tim@fs ❯ dua -V
dua 2.22.0                                                                                3ms
~
tim@fs ❯ dua emoji-test.txt
Error: Not a directory (os error 20)                                                                                3ms
~
tim@fs ❯

It works with wildcards on every version though:
~
tim@fs ❯ dua emoji-*
 188.00 KiB emoji-sequences.txt
 628.00 KiB emoji-test.txt
 816.00 KiB total

Thanks for letting me know! I will release a fix for it soon.

Great!
And here is the new release.


resin-mounts: Fixing typo

resin-mounts.inc are missing the "service"  to the systemd unit file of systemd-timesyncd.
Signed-off-by: Mans Zigher<EMAIL_ADDRESS>

Can one of the admins verify this patch?

@resin-jenkins ok to test


Add a tap for heroku

The Heroku install was not working for me until I made this change.
Fresh install on OS X High Sierra.

Thank you for this. Could you please also change the install line to be brew "heroku/brew/heroku"? That appears to be the recommended procedure going forward. I'll merge as soon as that's in.

@pablobm done! 👍

@jacobherrington Thank you!


[Confidential Ledger] Increment SDK Release

Description
Prepare Confidential Ledger patch release for October 2022.

API change check
API changes are not detected in this pull request.


[FlatList] does not recalculate number of visible rows after orientation change

Description
This was an issue posted by @Mtnt but it been closed recently because ListView is deprecated in 0.43. However, the issue still exists in flatList.
The original issue: #11218

Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally!
If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:

Does the issue still reproduce on the latest release candidate? Post a comment with the version you tested.
If so, is there any information missing from the bug report? Post a comment with all the information required by the issue template.
Is there a pull request that addresses this issue? Post a comment with the PR number so we can follow up.

If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution.


Improve dmverity-vhd -d performance

When using the -d option for dmverity-vhd, HTTP requests are made for each layer of the image. On a 2 core codespace runner, this takes roughly 10-15 seconds per layer, and ends up being the vast majority of the runtime especially for images with many layers.
To fix this, this PR instead uses the docker package to save an image into a tar file then follows the same code path as if the user used the -t flag to specify a local tar file. When v1.Layer.Uncompressed() is called, it just opens the file instead of making a new HTTP request via daemon.(*imageOpener).saveImage(), saving the request time of ~10-15 seconds per layer.
On the previously mentioned 2 core runner, this results in execution time going from ~150 seconds to ~20 seconds for a simple 9 layer image.

This PR is superseded by #2089 which doesn't need to compromise on disk space, I suggest we close this

Closed this as it's superseded by #2089


Iglu Webhook Adapter: allow schema to be passed on querystring for POST

At the moment it has to be inside the body. It might be useful to use the querystring to support certain third-party systems.

this is done, closing


When i run node index.js i got error can not find module index.js ?

module.js:471
throw err;
^
Error: Cannot find module 'F:\cezerin\index.js'
at Function.Module._resolveFilename (module.js:469:15)
at Function.Module._load (module.js:417:25)
at Module.runMain (module.js:604:10)
at run (bootstrap_node.js:394:7)
at startup (bootstrap_node.js:149:9)
at bootstrap_node.js:509:3
can you help me ou

Dear @useralive003,
Please click to Watch or Star this repository.
In two weeks I will

test on windows
deliver Docker image
write setup doc

Thanks for interest!

i did not get you , what are you trying to say

In couple weeks project will be ready to start on local environment.

now i cant use this ?

Right now project are in active development and not stable.
Soon I will release a demo and in month or two we will have stable release.

Please follow this step by step instructions to install.


app.block_inputs(true) does not block list actions

When I use app.block_inputs(true) on a form it doesn't disable the new button of list fields.
Is this a bug or am I using the function for the wrong purpose?
I am blocking the inputs while checking/waiting for the result of an async rest call from the backend.

Actions on list fields (like new, edit or delete) are not considered inputs, as they do not affect the record being handled by the current form.

That sounds plausible.
Maybe showing an overlay with a text or even a loading animation could help. It would prevent further interaction and signal a long running async process.
But maybe that is against your intended design.

It´s less about design intent and more about not being able to spend enough time to redesign larger components. Some things we can easily add to the current structure without breaking anything - other features require large redesigns and new concepts.
We are very aware of the limitations we currently have - we build a lot of REI3 applications for customers and also need to deal with what is possible and what is not. This is the case for any software of course - but it´s especially fun when you know that you could solve so many things with enough dev time.
But it´s good that people let us know, what features they are missing so that we can plan and prioritize accordingly. Please feel free to always share what is missing or sub-optimal for your use cases :)


Readme should be updated

Hello :)
Just to inform you about a small mistake. In the readme, there is:

For Swift project: you will need a bridging header, so just add a new file with Objective-C, confirm you want to a create a bridging header and just remove the not needed Objective-C file. In the bridging header, add: #import <EstimoteFleetManagement/EstimoteFleetManagement.h> For Objective-c project: add to your code #import <EstimoteFleetManagement/EstimoteFleetManagement.h>

It should be:

3 For Swift project: you will need a bridging header, so just add a new file with Objective-C, confirm you want to a create a bridging header and just remove the not needed Objective-C file. In the bridging header, add: #import <EstimoteFleetManagementSDK/EstimoteFleetManagementSDK.h> For Objective-c project: add to your code #import <EstimoteFleetManagementSDK/EstimoteFleetManagementSDK.h>

EstimoteFleetManagement => EstimoteFleetManagementSDK
Thank you :)

Fixed in https://github.com/Estimote/iOS-Fleet-Management-SDK/commit/867d4d83c429a80752a9de845a55a4bf107cb213 thanks for spotting this 🖖


2D

Can we have this but in 2d?

I know this is an old Issue but: Godot 4 will have a fastly improved TileMap that supports Scenes natively. You can already try it out with the alpha builds, though there will be bugs.


fix typos in swarm/cluster.go

fix typos in swarm/cluster.go
Signed-off-by: Sun Hongliang<EMAIL_ADDRESS>

LGTM


[Kiali][release-1.1] kiali needs to be told about the new istio-pilot port for /version

Describe the bug
It appears the latest Istio snapshot (1.1.0-snapshot.6) has changed the port istio-pilot is listening to for its /version endpoint. This endpoint is used by Kiali to determine what version of Istio it is talking to. The old port is no longer active and thus causing Kiali to fail in places. See: https://issues.jboss.org/projects/KIALI/issues/KIALI-2403 and https://issues.jboss.org/browse/KIALI-2411
Expected behavior
Kiali to be able to access istio-pilot's previous /version endpoint so it can figure out what version of Istio it is deployed in.
Steps to reproduce the bug
Go to Kiali's /api/status endpoint and notice a failure. You should see some JSON with status information, including the Istio version. This also causes problems in the Kiali About Box - it actually screws up all the data in the About Box, so now there won't be a way to know about the environment Kiali is deployed in (Kiali's own server-side version string will not show up, as well as versions of things like k8s and istio, etc).
Version
Istio 1.1.0-snapshot.6 and any Kiali release. It appears this only just happened with the latest snapshot.6 - we did not see this problem in any previous 1.1 snapshots.
The Fix
Here's how Kiali is fixing this: https://github.com/kiali/kiali/pull/851/files#diff-3baf47c64847a8fb8aaa8cc2e088513b
You can see previously it was talking to http://istio-pilot:9093/version but that port 9093 is no longer active, Kiali now needs to talk to http://istio-pilot:8080/version.
For Istio 1.1, we do not necessarily have to bump up to a new version of Kiali (in fact, there is no Kiali release today that fixes that code - that PR was only just merged today so we need the next release to pick up that fix). However, this URL is overrideable through the configmap or an environment variable. I think the easiest thing we could do now (to get this into Istio 1.1) is to change the Kiali deployment or configmap such that we configure it to use the new pilot URL. Something like this in Kiali's deployment.yaml env section:
- name: ISTIO_URL_SERVICE_VERSION
  value: http://istio-pilot:8080/version

or in the configmap with this setting:
external_services:
  istio:
    url_service_version: http://istio-pilot:8080/version

This should be a quick and easy fix, with low risk. I think we should consider getting this into Istio 1.1.
Thoughts?
cc @linsun
Here's a PR that should address this: https://github.com/istio/istio/pull/11833

What the Kiali About Box should look like (yes the Jaeger and Grafana URLs are bogus - I don't have them installed - I actually configured Kiali with those bogus URLs):

What it looks like with this bug:

[side note: We plan on hardening Kiali so this isn't so drastic if the istio version fails to be obtained, but that fix will come in the future - that work will be done as part of KIALI-2411]

PR https://github.com/istio/istio/pull/11833 merged. We can close this now.


refactor(index): refactored text area for post mumble on index page

You could also add translations in this branch

added the missing translations for the index page


Added Skclusive.Material.Components 'cause it's awesome

Take one look at the demo/docs, it is a great library and absolutely deserves to be on the list.

Component List
Dashboard

I squashed the multi-commit into one.

Thank you very much!


No warning is given if commitlint.config.js does not exist

Expected Behavior
If the commitlint.config.js file does not exist then commitlint does not have rules to check against and does nothing. It should display a warning message, e.g. "Warning: configuration not found, exiting".
Current Behavior
When the commitlint.config.js file does not exist then commitlint does nothing, but also does not warn the user.
Affected packages

[x] cli
[ ] core
[ ] prompt
[ ] config-angular

Possible Solution
Check if the configuration with ruleset is available, if not then display a warning message, e.g. "Warning: configuration not found, exiting".
Steps to Reproduce (for bugs)
N/A
Context
It has happened two me twice know that I installed everything and forgot to create the config file and only discovered my omission later. In one case it meant a lot of wrong commit messages :-(
Your Environment
N/A

Should be covered by #107 (#491 )

Cool. Didn't see it. Any idea when it will be merged?

It's merged but not yet released. Not sure when the new release will come.

Great. I will be on the lookout for it :-)

Cool. Didn't see it. Any idea when it will be merged?

It's merged but not yet released. Not sure when the new release will come.

We are in the process of distributing maintenance of commitlint between @escapedcat, @byCedric and me. I am a bit swamped with work recently so this drags out a bit. I am confident we'll do a feature release in the first two weeks of 2019 though 🤖


Fix Foundry test suite

Previously, it was not possible to run the existing Foundry test suite. This PR makes a couple of modifications to both the tests and foundry.toml to get the whole suite running, with a view to contributing additional improvements based on the testing methodology employed by Sablier V2.

This has been implemented in #909.


landmark detection

Hi, davisking:
I would like to ask a problem about the face landmark detection , I try to optimize themodel size, hoping to drop to 5M and below, may I ask some feasible direction?
Thanks a lot.

Train a new model with fewer landmarks or smaller trees.

I optimize the model to 18M. in the way of store all the leaf into a packed format.
which can be optimized more.
about 10M I think.

Hi, @9crk
Did you optimize the model to 18M by retrain a new model or something else?
"Store all the leaf into a packed format"
Could you please give me more details about it?
Thanks.

@laper just store all the value into a binary file.(not a ascii file). then it works.


Swap UI [part 1]

Re-enable frontend tab
New denom-out UI component (non-editable)
planWitnessBuildBroadcast should be a shared function between send/ibc/swap
Implement proper Denom Registry. Related: https://github.com/penumbra-zone/web/issues/140
Add swap/swap commitment to view-protocol-server/transaction-planner.ts

lpnft asset display names look pretty ugly in the ui right now

Can you share a screenshot here? @turbocrime


Add total support for I18N and for custom first weekday selection

Hi,
At first time, congratulations for this amazing component. I tried to make a similar component but you made it better than me xD
I did two commits to improve your calendar component:

Add total support for I18N, including month names. Set "en" as default language. In case you don't add the customI18n object, or any property is not included, a default english value will be used.
Allow to select the first weekday for calendar (month days and week names). For example in Spain the first weekday is Monday. With these changes you can select any day as first weekday.

I hope you like these two commits.
Regards

Oh sorry, travis failed because I forget to adapt the tests.

Coverage decreased (-1.3%) to 89.12% when pulling 342eb9bd71c0f937c928430251ae7e341f6192f7 on giraldezjorge:master into acd6b0b7a7b1e58185d6a912b0e9441838a1fbe5 on Tinysymphony:master.

I fixed tests. They passed now

Coverage decreased (-1.6%) to 88.813% when pulling 527491a866b3e6806fd6ff0e611f3e89d6beb881 on giraldezjorge:master into acd6b0b7a7b1e58185d6a912b0e9441838a1fbe5 on Tinysymphony:master.

Coverage decreased (-1.5%) to 88.939% when pulling 9297914c4e053209abc601ec43a63bdf4546d1a8 on giraldezjorge:master into acd6b0b7a7b1e58185d6a912b0e9441838a1fbe5 on Tinysymphony:master.

Coverage decreased (-4.7%) to 85.745% when pulling 2ed1bf88c9a334520e757030e6877cc7c169ee07 on giraldezjorge:master into acd6b0b7a7b1e58185d6a912b0e9441838a1fbe5 on Tinysymphony:master.


tenki: 1.4.0 -> 1.5.0

Automatic update generated by nixpkgs-update tools. This update was made based on information from https://github.com/ckaznable/tenki/releases.
meta.description for tenki is: tty-clock with weather effect
meta.homepage for tenki is: https://github.com/ckaznable/tenki
Updates performed

Rust version update

To inspect upstream changes

Release on GitHub

Compare changes on GitHub

Impact
Checks done

built on NixOS
The tests defined in passthru.tests, if any, passed
found 1.5.0 with grep in /nix/store/iim9k5z9d6bslv1wrsm5vjg9ria6w2a5-tenki-1.5.0
found 1.5.0 in filename of file in /nix/store/iim9k5z9d6bslv1wrsm5vjg9ria6w2a5-tenki-1.5.0

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

1 total rebuild path(s)

1 package rebuild(s)

First fifty rebuilds by attrpath
tenki

Instructions to test this update (click to expand)

Either download from Cachix:
nix-store -r /nix/store/iim9k5z9d6bslv1wrsm5vjg9ria6w2a5-tenki-1.5.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 tenki https://github.com/r-ryantm/nixpkgs/archive/8fec083b833146eba481d7f4175f8d386e5b056a.tar.gz

Or:
nix build github:r-ryantm/nixpkgs/8fec083b833146eba481d7f4175f8d386e5b056a#tenki

After you've downloaded or built it, look at the files and if there are any, run the binaries:
ls -la /nix/store/iim9k5z9d6bslv1wrsm5vjg9ria6w2a5-tenki-1.5.0
ls -la /nix/store/iim9k5z9d6bslv1wrsm5vjg9ria6w2a5-tenki-1.5.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.
Result of nixpkgs-review --extra-nixpkgs-config '{ allowInsecurePredicate = x: true; }' run on x86_64-linux 1

1 package built:

tenki

Maintainer pings
cc @iynaix for testing.

@NixOS/nixpkgs-merge-bot merge


Accept Path as well as str in read_input() and read_input_group()

populationgenomics/production-pipelines#727 was yet another example of us forgetting to call str() on a CloudPath before passing it to read_input(). I do this regularly too, but usually catch it (with great tedium) in local testing.
I'm not sure there's any good reason why we do this to ourselves! Seems like there'd be no conceptual reason why these methods shouldn't take a path-like equivalently to a str, so this PR suggests overriding these methods in our hb.Batch subclass so that they will accept either str (as per the superclass) or additionally CloudPath/pathlib.Path.
(It would be less useful to push this upstream to mainstream Hail, as it doesn't already use cloudpathlib so they would probably prefer to only support str | pathlib.Path. But I suppose an alternative to this subclass addition would be to get that much merged upstream and then carry … | CloudPath in our local Hail patches…)

Actually maybe the upstream Hail change that would render this overriding unnecessary would be
--- a/hail/python/hailtop/batch/batch.py
+++ b/hail/python/hailtop/batch/batch.py
@@ -466,7 +466,7 @@ class Batch:
         self._resource_map[jrf._uid] = jrf  # pylint: disable=no-member
         return jrf

-    def read_input(self, path: str) -> _resource.InputResourceFile:
+    def read_input(self, path: str | os.PathLike) -> _resource.InputResourceFile:
         """
         Create a new input resource file object representing a single file.

@@ -495,7 +495,7 @@ class Batch:
         irf = self._new_input_resource_file(path)
         return irf

-    def read_input_group(self, **kwargs: str) -> _resource.ResourceGroup:
+    def read_input_group(self, **kwargs: str | os.PathLike) -> _resource.ResourceGroup:
         """Create a new resource group representing a mapping of identifier to
         input resource files.

along with a suitable (check and) conversion to str, perhaps in _new_input_resource_file().
Both pathlib.Path and CloudPath are path-like as measured by os.PathLike, so this would do the trick for cloudpathlib without Hail needing to mention it directly.

The upstream version of this has now been merged upstream: see hail-is/hail#14544. I would be tempted to cherry-pick that into our current Hail uplift so that we don't have to apply this version to our subclass after all.

@jmarshall +1 to cherry picking this commit (or merging up to it!)

The upstream version of this appeared in 0.2.131 so it has been available in our installation for some time. So this PR is no longer necessary.


[BUG] materialize_features fails with Spark error: java.lang.IllegalAccessError

Willingness to contribute
No. I cannot contribute a bug fix at this time.
Feathr version
All versions
System information
Databricks runtime 10.4
Describe the problem
Call materialize_features with redis sink, using maven jar file. The spark job in Databricks is failing with java.lang.IllegalAccessError
This seems caused by mismatch protobuf between feathr build.sbt (v3) and spark cluster (v2).

protobuf-java used in feathr build.sbt is 3.19.4, source:https://github.com/linkedin/feathr/blob/main/build.sbt#L42
protobuf-java used in Azure Databricks is 2.6.1, source: https://docs.databricks.com/release-notes/runtime/10.4.html
protobuf-java used in Azure Synpase is 2.5.0, source: https://docs.microsoft.com/en-us/azure/synapse-analytics/spark/apache-spark-3-runtime

Only user who use runtime jar from maven will have impact, this issue does not occur if user uses runtime from Azure Storage which is a flatten jar.
Detail logs (full log can be viewed at https://adb-1996253548709298.18.azuredatabricks.net/?o=1996253548709298#setting/clusters/0826-011329-8niddv06/driverLogs:
Py4JJavaError: An error occurred while calling z:com.linkedin.feathr.offline.job.FeatureGenJob.mainWithPreprocessedDataFrame. : org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 13.0 failed 4 times, most recent failure: Lost task 0.3 in stage 13.0 (TID 20) (<IP_ADDRESS> executor 1): java.lang.IllegalAccessError: tried to access field com.google.protobuf.AbstractMessage.memoizedSize from class com.linkedin.feathr.common.types.protobuf.FeatureValueOuterClass$FeatureValue 	at com.linkedin.feathr.common.types.protobuf.FeatureValueOuterClass$FeatureValue.getSerializedSize(FeatureValueOuterClass.java:1296) 	at com.google.protobuf.AbstractMessageLite.toByteArray(AbstractMessageLite.java:64) 	at com.linkedin.feathr.offline.generation.outputProcessor.RedisOutputUtils$.$anonfun$getConversionFunction$2(RedisOutputUtils.scala:99) 	at com.linkedin.feathr.offline.generation.outputProcessor.RedisOutputUtils$.$anonfun$encodeDataFrame$2(RedisOutputUtils.scala:51) 	at com.linkedin.feathr.offline.generation.outputProcessor.RedisOutputUtils$.$anonfun$encodeDataFrame$2$adapted(RedisOutputUtils.scala:48)
Tracking information
No response
Code to reproduce bug
No response
What component(s) does this bug affect?

[ ] Python Feathr Client: This is the client users use to interact with most of our API. Mostly written in Python.
[X] Computation Engine: The computation engine that execute the actual feature join and generation work. Mostly in Scala and Spark.
[ ] Feature Registry API Layer: The storage layer supports SQL, Purview(Atlas). The API layer is in Python(FAST API)
[ ] Feature Registry Web UI layer: The Web UI for feature registry. Written in React with a few UI frameworks.

The issue is caused by a dependency version mismatch between Feathr core and DataBricks Runtime.
The Feathr core uses Google ProtoBuf 3 to encode Redis values but DataBricks provides ProtoBuf 2 in their runtime environment.
This bug will happen when:

Running Feathr jobs on DataBricks, and,
Using Redis as the output sink, and,
Using Jar from Maven instead of local or other sources.

NOTE: The bug will not be triggered if the job uses the fat JAR, it only affects Maven JAR because the JAR from Maven doesn’t include its dependencies inside of the JAR file, it relies on the environment to provide/resolve dependencies.
The workaround is to use the fat JAR, either from our public storage, or user can build one by themselves.
One possible solution is to downgrade the version of the ProtoBuf to 2 in Feathr core, I did some preliminary tests and found it should work, but we still need wider tests to confirm all related functionalities.

Thanks for debugging and sharing your insights.
Let are some incompatibilities between 2 and 3. Before switching, we should figure out those.
Besides, was wondering if anyone know any way to keep 3 even the cluster is using 2?
This might be a backward incompatible changes for people who already pushed data to redis with 3.
We should also think about what's our strategy in the future when the cluster start to use 3.

Lower feathr runtime protobuf version from 3 to 2 is one option, i am bringing some other options for discussion

Include protobuf 3 into feathr runtime jar.
Not use protobuf, just save json string to redis. Actually for other store like cosmos db, aerospike, protobuf is not used, same applies to Elastic Search which is coming soon. Probably there some known limitation in Redis that we have to use protobuf, pls let me know.

i agree on backward incompatible issues, we should make sure existing data written by protobuf3 can still be consumed.
cc @xiaoyongzhu

@blrchen those are good options we can consider.
Redis, and some other databases, only supported primitive data types whereas Feathr has richer data types, like list and map. So encoding is needed here. We can also use other encoding libs, like Avro, but the same version incompatiblity may exist. In LinkedIn, we use Avro and we also face the same issue. For avro, i remember we have a compatibility layer to convert between 1.4 and 1.7. It seems possible for protobuf 2 and 3 as well.
Do you save json to cosmos db? I am not sure if the performance is optimal though. If that's the recommended approach, that should be fine. If not, we can conduct some study to figure out the optimal encoding we want to use.


[DRAFT PR]POC code to batch async shards fetch per node

Description
This is the the draft PR to discuss on the idea of batching the AsyncShardFetch used in GatewayAllocator.
In the current flow of code  PrimaryShardAllocator/ReplicaShardAllocator(PSA/RSA) calls GatewayAllocator to do  Transport calls per shard to N data nodes for async fetching the metadata of shards from all nodes. This information is then used by PSA/RSA to make allocation Decision. Currently GatewayAllocator maintains two such maps asyncFetchStarted and asyncFetchStore to store async data
So we make M(number of unassigned shards)*N(number of data nodes) transport calls for assigning Unassigned Shards.
The current POC is done to batch the async shards Transport call per node and maintain that view of shards metadata from all nodes in GatewayAllocator.
The changes tries to replace one current map asyncFetchStarted whose data(TransportNodesListGatewayStartedShards.NodeGatewayStartedShards) is used in PSA primarily, with shardsPerNode (naming can be done better here though) to maintain the same set of data  in list(TransportNodesCollectGatewayStartedShard.ListOfNodeGatewayStartedShards) per node.
Please note the changes are done in crude way(just to convey idea) with reusing of code from current classes and are draft changes
Flow diagrams:

Current overview picture of how and why asyncFetchShards come into picture:

More simpler view:

Whats changing:

List of changes:

GatewayAllocator

Added a map shardsPerNode to replace asyncFetchStarted.  The map will contain the current view of fetched data of unassigned shards from all nodes. This will then be used to send the data after adaptation to PSA
fetchShardsFromNodes. Is a object that maintains the current fetch status from the nodes in cache. This is analogous to AsyncShardFetch
In beforeAllocation() added  collectShardsPerNode(allocation) as a responsibility. beforeAllocation() is called by AllocationService before starting the allocation. This will help in populating the shardsPerNode and when the allocation decision needs to be taken in allocateUnassigned, the data can be used readily by PSA.
collectShardsPerNode(). Task to build the view of shards MD from all nodes
TestAsyncShardFetch. Inner class analogous InternalAsyncFetch
TestInternalPrimaryShardAllocator.  Inner class analogous InternalPrimaryShardAllocator. ThefetchData()here do not actually does the heavylifting of fetching, rather adapts the current data in shardsPerNode to the AsyncShardFetch.FetchResult that PSA understands. This currently helps to minimize the scope of changes in POC. We can later accommodate to revamp PSA/RSA to make allocationDecision based on the batched data.

Added New Transport action TransportNodesCollectGatewayStartedShard, analogous TransportNodesListGatewayStartedShards. This is added for just for simplicity of code and we can think of merging it to a single transport action

Added a new AsyncShardsFetchPerNode class to keep do a track async fetch of shards(shardsToCustomDataPathMap) from all nodes. It is analogous to AsyncShardFetch class which does per the same thing per shard.We can also later remove that class or merge this class into AsyncShardFetch depending on the POC approval

Testing
Did a happy test manually by spinning two data nodes and a single master with 16 shards. Restarted the data nodes, shards turned from Unassigned to Assigned .
Also tested on number of transport calls made by adding a log line in nodeOperation, can verify that a single transport call per node is made(if unassigned shards set is not updated). So thats reduction of transport calls from M*N to N.
Issues Resolved
https://github.com/opensearch-project/OpenSearch/issues/5098
This might be extension of the above issue since it might not solve completely the memory issue, but solves the transport calls optimization part.
Check List

[ ] New functionality includes testing.

[ ] All tests pass

[ ] New functionality has been documented.

[ ] New functionality has javadoc added

[ ] Commits are signed per the DCO using --signoff
[ ] Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@shwetathareja @amkhar Thanks a ton for taking time in reviewing and providing valuable insights on it.

We should change the strategy at both of the calls not just one.

@amkhar We cant do this, One calls is used for primary Shards allocation and other one in RSA. Currently the algo first assigns primary and then replicas

Without changing ReplicaSHardAllocator how are we ensuring that metadata fetch is done on node level ?

@amkhar  The code changes are not for RSA right now. The focus is on PSA. We will add a new transport Action like we added for the data that PSA needs.

AllocationService is making calls in a for loop for every shard for allocateUnassigned method which calls GatewayAllocator then ultimately PSA's makeAllocationDecision is called. I want to understand how this flow is being per node without changing AllocationService at all ?

So the idea is currently AllocationService once calls beforeAllocation() here
I am using this to create the view(the data PSA needs) here
This way when GatewayAllocator when AllocationService  Calls GatewayAllocator for assigning the shards, we already have the data collected(if not, then we just bail out that allocation) and when PSA calls fetchData to makeAllocation Decision we return the data from our view of data by adapting to what PSA currently understands here
And as  @shwetathareja visual diagrams needs to be added for better understanding. I am working on it and will soon add.

Please try to convey the impact of the change as in how are we benefitting from reduced number of transport calls with almost similar amount of data ?

Do you have any idea how to convey that? General observation have been in large clusters with high number of shards Transport gets choked due to the async Fetches. With this change there will be less stress on transport.

Let's try to not change the order of flow like whatever is happening after allocation let it happen in that order only and same for before allocation.

There's nothing changing afterAllocation, we are using it to clear our caches and DS like existing code is also doing. In before allocation we are adding a code to populate the view in the GatewayAllocator. Since AllocationService calls GatewayAllocator for all unassigned shards in loop, we dont want to populate the view(collectShardsPerNode) for every call. That's why its better to keep the view even beforeAllocationStarts(@shwetathareja  your answer to inline comment for it).

4. Please add a diagram which can capture the flow of calls across classes and method to understand easily.

@shwetathareja Tried to capture the details in the flow diagram, Updated the description


std.container.Array: Payload "default constructor" simplifies Array.

The purpose is reduce code duplication.
Thank you for considering this.

Love seeing a red diff!

Thank you for constructive and encouraging review.
This PR is a continuation of https://github.com/dlang/phobos/pull/8143. Both PRs have been motivated by seeing repeated code, similar to Payload.reserve.

But maybe someone more familiar with Array than me should check whether the duplicated code didn't have subtle differences that were important e.g. for performance.

Such checks (and all constructive reviews) are very welcome !
For now, I have tried to fix the performance problem spotted in your review (insertBack redundantly checking length vs capacity).

The buildkite failure might be fixed by a rebase.

The buildkite failure might be fixed by a rebase.

Thank you. Let us see !

I am very curious as to why Data.init cannot be used (https://github.com/dlang/phobos/pull/8162#discussion_r668377463).
This comment is just to inform that, currently, with my low skills and high laziness, it is going to take some time to investigate.

@DoctorNoobingstoneIPresume Any updates on this?

@DoctorNoobingstoneIPresume Any updates on this?

@RazvanN7 @pbackus I apologize for the delay. I have only today resumed investigations -- currently I am trying to better understand RefCounted. I hope to answer again soon.

i.e. what is it about RefCounted ! (...) which makes a constructor with an empty body differ in result from .init.

In my experience the most likely cause of this is a nested struct.
Nested structs have a hidden context pointer that must be initialized at runtime. When you construct an instance using constructor syntax (e.g., MyStruct()), this initialization is performed. However, when you use .init, this initialization is skipped, and the context pointer is set to null instead. Here's an example:
unittest
{
    int x = 1;

    struct Nested
    {
        int y = 2;
        int fun() { return x + y; } // access context
    }

    Nested a = Nested(); // use constructor syntax
    assert(a.fun() == 3); // ok

    Nested b = Nested.init; // use .init
    assert(b.fun() == 3); // Segmentation fault
}

The reason for this is that the .init value of a type is always evaluated at compile time, not runtime.


⚠ Bofrost.be has degraded performance

In 72e2d1f, Bofrost.be (https://www.bofrost.be) experienced degraded performance:

HTTP code: 200
Response time: 1751 ms

Resolved: Bofrost.be performance has improved in 565dc0b.


Triggered vs SaveChangesInterceptor

Hi
Sorry if it is not the right way to ask questions but dont really know which other way to contact you.
I am wondering how this project is compared to SaveChangesIntercepter? Is there any reason to use this over SaveChangesInterceptor? Would be great with a comparison in the wiki if this project is still wort using.
Also do you recommend it for production ?

SaveChangesInterceptors are great for simple tasks like setting a created date or implementing soft-deletes. this project uses SaveChangesInterceptors under the hood and solve some common problems with SaveChangesInterceptors:

Triggers support dependency injection. You can inject your own services within your trigger instances that are then resolved from the same service scope as the one used to obtain an instance of the current DbContext.
Triggers are recursive. Modifying the entity graph in a trigger can result in subsequent triggers getting invoked.
Triggers provide an simplified programming model. Instead of implementing an interceptor and looking at the changed entities in there, you can just implement a trigger for a particular entityType (or interface).

I recommend looking at the StudentManager sample in this repo for a use-case

IAfterSaveChanges will not automatically call SaveChanges again. You'll have to manually call SaveChanges again if this is intended. You could consider a trigger that implements IAfterSaveCompletedTrigger to perform another SaveChanges call. This will be a no-op if there are no additional changes to save. Otherwise it will raise triggers accordingly. That said, It sounds like you would want to use IBeforeSaveTrigger instead as it allows you to manipulate your entity graph when saving

Thanks a lot


Max ending interval

I'm attempting to get the interval that has the maximum end value. I seem to wrongly have assumed the Max function would return this.
  func Test_IntervalTree(t *testing.T) {
      f := func(v1, v2 int64) int {
          if v1 < v2 {
              return -1
          } else if v1 > v2 {
              return 1
          }
          return 0
      }
      tree := interval.NewMultiValueSearchTreeWithOptions[int64](f, interval.TreeWithIntervalPoint())

      require.NoError(t, tree.Insert(5, 10, 30))
      require.NoError(t, tree.Insert(0, 100, 7))

      v, _ := tree.Max()
      fmt.Println("Max: ", v)
      v, _ = tree.Min()
      fmt.Println("Min: ", v)
  }

Max:  [30]
Min:  [7]
PASS

Is there a way to find the maximum ending interval?

I should've documented better the Max method. It's supposed to return the values of right-most interval stored on the tree, not necessarily the interval with the maximum right endpoint value.
I understand your use case, tho. I will review your PR asap.
Thank you.

PR #8 merged. I will be releasing a new version today. Thank you again for your contribution.


203 set dimension bound for dynamic shape

set dimension bound for dynamic shape

What is the purpose of this change? Does it decrease memory usage?

What is the purpose of this change? Does it decrease memory usage?

this is the method we official recommend to implement dynamic shape:
https://docs.openvino.ai/latest/openvino_docs_OV_UG_DynamicShapes.html#dimension-bounds


Cluster teardown for GCE failing with an error from Managed Instance Groups

My perception is that since we moved to Managed Instance Groups the reliability of brining clusters up and tearing them down for GCE has gone down. And I say that as someone who usually creates and destroys many GCE clusters every day (sometimes quite large ones).
$ cluster/kube-down.sh
Bringing down cluster using provider: gce
Project: kubernetes-satnam2
Zone: us-central1-b
ERROR: (gcloud.compute.firewall-rules.delete) Some requests did not succeed:
 - The resource 'projects/kubernetes-satnam2/global/firewalls/kubernetes-fluentd-elasticsearch-logging' was not found

current-context: "kubernetes-satnam2_kubernetes/"
Running: cluster/../cluster/gce/../../cluster/../cluster/../cluster/gce/../../cluster/../_output/dockerized/bin/darwin/amd64/kubectl delete services elasticsearch-logging
F0314 00:36:03.809132   77311 delete.go:67] Client error processing command: Get http://localhost:8080/api/v1beta1/services/elasticsearch-logging?namespace=default: dial tcp <IP_ADDRESS>:8080: connection refused
current-context: "kubernetes-satnam2_kubernetes/"
Running: cluster/../cluster/gce/../../cluster/../cluster/../cluster/gce/../../cluster/../_output/dockerized/bin/darwin/amd64/kubectl delete services kibana-logging
F0314 00:36:03.862686   77330 delete.go:67] Client error processing command: Get http://localhost:8080/api/v1beta1/services/kibana-logging?namespace=default: dial tcp <IP_ADDRESS>:8080: connection refused
Bringing down cluster

ERROR: (gcloud.preview.managed-instance-groups.delete) ResponseError: code=404, message=Not Found

Stuck at this step for over an hour.
$ cluster/kube-up.sh
Starting cluster using provider: gce
... calling verify-prereqs
... calling kube-up
Project: kubernetes-satnam2
Zone: us-central1-b
current-context: "kubernetes-satnam2_kubernetes/"
Running: cluster/../cluster/gce/../../cluster/../cluster/../cluster/gce/../../cluster/../_output/dockerized/bin/darwin/amd64/kubectl config view -o template --template={{$root := .}}{{with index $root "current-context"}}{{with index $root "contexts" .}}{{with index . "user"}}{{with index $root "users" .}}{{index . "username"}}{{end}}{{end}}{{end}}{{end}}
current-context: "kubernetes-satnam2_kubernetes/"
Running: cluster/../cluster/gce/../../cluster/../cluster/../cluster/gce/../../cluster/../_output/dockerized/bin/darwin/amd64/kubectl config view -o template --template={{$root := .}}{{with index $root "current-context"}}{{with index $root "contexts" .}}{{with index . "user"}}{{with index $root "users" .}}{{index . "password"}}{{end}}{{end}}{{end}}{{end}}

Creating gs://kubernetes-staging-a2dc4
Creating gs://kubernetes-staging-a2dc4/...

That last one is completely unrelated to MIGs - the CreateBucket failed. Which is also a rare operation - is this the first time you've used this project for k8s? Otherwise the bucket would be there.
I'm not sure what's different about your environment, but Jenkins also churns through a cluster every half hour and rarely sees issues. It's a stat I want to get more precision on, but I don't think MIGs are to blame. You probably also tend to do things like switch projects often, switch networks often, etc - all of these things add up in making the tools more brittle, I suspect.

In that case I shall just close this issue since I can't see what the resolution might be.


Comments updated

Sorry For the Inconvenience.
Although it was nice adding the comments in the code it was not suggested by the Hacktoberfest. They will mark the Repo as a Spam then every pull request that has been made to this repo will become invalid.
So, I suggest you not to add or update the comments. Kindly add the files that you worked on and update the details in the readme.
Regards
Manjunath


Implement lampager-cakephp

🎉Happy New Year 🎉
It's lovely to release a brand new product from Lampager team! This PR contains the solid implementation of the CakePHP adaptor of lampager that is tested with the both MySQL and SQLite. Most of the functionalities are covered with the tests currently written with some remaining part whining uncovered seen below.
Features

CakePHP integration

[x] for Table via Lampager\Model\Behavior\LampagerBahavior
[x] for Controller via Lampager\Datasource\Paginator

Tests

[ ] Fully-covered unit tests
[x] Mostly-covered unit tests with the unexceptional scenarios

Database Support

[x] MySQL
[x] SQLite (see note below)

Coverage

Note
A special SqliteCompiler is bundled in order to let it successfully compile UNION ALL with ordered SELECT statements because the query builder of the CakePHP 3.x does not handle that case for SQLite.

@mpyw Thanks for reviewing! Merging this now and adding more tests to cover the remaining part later on.


External CSS

Im trying to write an application which converts HTML to PDF.
I have tried to cram it all in one string HTML and CSS but it does not seem to like it.
I noticed there is a way to add external CSS to the generatepdf command, however i have no idea how to create the CSSdata properly, does anyone know? i cannot seem to find any documentation on it either..

Hi there
The PdfGenerator.cs class has a static method ParseStyleSheet which returns the required CssData, generated from a string.
https://github.com/ArthurHub/HTML-Renderer/blob/cc3b5c221b192a950d3d53b561ac01049668e239/Source/HtmlRenderer.PdfSharp/PdfGenerator.cs#L56
Is that what you were looking for?
Best

Ah yes perfect! exactly what i was looking for.

Can any of you guys/girls  provide an example on how to use the above method?
This is what I'm currently doing:
string testStylesheet = "~/Scripts_RMS/Shared/FormLetter/ckeditor.css"; var cssData = TheArtOfDev.HtmlRenderer.PdfSharp.PdfGenerator.ParseStyleSheet(testStylesheet, true); doc = PdfGenerator.GeneratePdf(m_Html, GetConfig(nPageSize), cssData);
Any thoughts?
Cheers,
Adhy

I wrote the CSS in a string, i didnt feed it a file

So you did something similar to this?
string testStylesheet = "<style type="text/css">p { line-height: 1!important; }</style>";  var cssData = TheArtOfDev.HtmlRenderer.PdfSharp.PdfGenerator.ParseStyleSheet(testStylesheet, true);

without the style tag, plain CSS
i had something like
string css = ".img { position: something; }" +
".otherclass { color:white; }";
Something like that, i dont have my source handy right now

Thanks, I'll give this a go. Have you played around with line-height CSS by any chance? It seems that the PDF converter doesn't work for pt values, though I'm not 100% sure.

Hi, I have tried line-height using pt values. But it seems to ignored it.

hi guys, any solution for this post?


Add Hot Corners Like in MacOS and Gnome

Description of the new feature / enhancement
Hot Corner to Open Notes app or Overview Mode on Top-Left corner
Scenario when this would be used?
Better For Mouse Driven Application(s) uses, as fewer clicks will be required to switch apps and workspaces.
Supporting information
No response

/dup #1305


Cache OnActivityResult Classes to use only one reflection call per class

Current coverage is 93.76%

Merging #39 into master will decrease coverage by -0.27% as of 29fd30f

@@            master     #39   diff @@
======================================
  Files           15      15
  Stmts          604     609     +5
  Branches       121     122     +1
  Methods          0       0
======================================
+ Hit            568     571     +3
- Partial         18      19     +1
- Missed          18      19     +1

Review entire Coverage Diff as of 29fd30f

Powered by Codecov. Updated on successful CI builds.


[Refactor] Implement versions catalog classes into the conventions submodule.

You are using libs.findLibrary("someLib").get() in some convention plugin classes to implement the dependencies. Wouldn't it be better if this was libs.someLib like in the library/module build.gradle files?
I guess we can obtain this capability by including version catalog-related classes in the conventions module like this:
// VersionCatalog classes.
implementation(files(libs.javaClass.superclass.protectionDomain.codeSource.location))

By adding these files, we can get rid of the noise that the .findLibrary().get() function is adding. Also, we get type-safe dependency providers like this example below:
dependencies {
  add("testImplementation", libs.findLibrary("robolectric").get())
  add("testImplementation", libs.findLibrary("roborazzi").get())

  // Type-safe, and much less noise in code.
  val libs = the<LibrariesForLibs>()
  add("testImplementation", libs.robolectric)
  add("testImplementation", libs.roborazzi)
}

I've seen this practice in Signal-Android repository.

👋 Hi @nymexe, this looks like a duplicate of:

#317
#318
#937


LS shuts down gracefully when connection to RabbitMQ breaks

I have a setup where Logstash connects to RabbitMQ server through a Keepalived VIP -> Haproxy loadbalancer which works perfectly fine but i have been doing some component test failures and found an issue.  My Keepalived checks for HaProxy running and if it is not then drops the weight of the server and the VIP moves to the slave and that HaProxy then continues.
The test i did was to stop HaProxy on the primary and monitor the VIP move but i noticed that the 2 Logstash instances that connect to Rabbit through this both shutdown without error.
I started Logstash with -v and ran the test again and got the following:
Pipeline started {:level=>:info}
Logstash startup completed
Input plugins stopped! Will shutdown filter/output workers. {:level=>:info}        <-- haproxy stopped
Pipeline shutdown complete. {:level=>:info}
Logstash shutdown completed

Should the input not be able to handle this or am i missing a config setting?  I cannot find anything in the documentation for the Rabbit Input.
Thanks,
Simon

Hmmm, this should no longer be the case. Which version of the rabbitmq plugin are you using?. You can check your Gemfile with cat Gemfile.jruby-1.9.lock | grep input-rabbitmq.
This should be fixed in 3.1.4. If not please let me know and I'll look into it. If upgrading fixes it please close the issue.

Hi Andrew,
[landerson@elk-logstash04 ~]$ sudo cat /opt/logstash/Gemfile.jruby-1.9.lock | grep input-rabbitmq
logstash-input-rabbitmq (3.1.3)

logstash-input-rabbitmq
[landerson@elk-logstash04 ~]$
This is the plugin that came with logstash 2.2.  I will get this upgraded and let you know.
Thanks.

It seems like Logstash 2.2.1 came to the rescue with 3.1.4 :)
Cheers Andrew, all working now.

Hey @thenom you may want to upgrade to 3.1.5. There was an issue with duplicate consumers in 3.1.4 that I fixed :(. Sorry for the churn!

Ah, ok.  Cheers for letting me know.  Appreciated.

Hi Andrew,
I have noticed on the new version (3.1.5) that the throughput is greatly reduced, even though the consumers bug has appeared.  with my current setup i could get over 10K message collected per second but with 3.1.5 i can only get approx 2K.  Is this a known issue or do you want me to open another?
Thanks,
Simon


Two for-loops

https://github.com/de4a-wp5/de4a/blob/d47997f6deda975bc61f036432c1494e96ae3276/de4a-connector/src/main/java/eu/de4a/connector/api/manager/ResponseManager.java#L78
Two for-loops? This can (and should) be refactored to one

The two for loops are still there. One is empty

buah, nice fix that one...


contracts migration

Multi-block migration framework for pallet-contracts.
Test migration
Steps to test against contracts-rococo:
#1 build and run cumulus and move the binary to /usr/local/bin/polkadot
cargo build  --release
mv target/release/polkadot-parachain /usr/local/bin/polkadot

#2 sync it, and keep it running in one terminal tab
polkadot \
	--chain contracts-rococo \
	--base-path ~/Documents/db/contracts-rococo

#3 compile cumulus against local checkout, and build it with try-runtime feature
diener patch --crates-to-patch /path/to/substrate --substrate
cargo build --features try-runtime --release

#4 run the migration with
RUST_LOG="runtime::contracts=debug,try-runtime::cli=debug" ./target/release/polkadot-parachain try-runtime \
	--runtime target/release/wbuild/contracts-rococo-runtime/contracts_rococo_runtime.wasm \
	--chain contracts-rococo \
	on-runtime-upgrade live \
	--uri ws://localhost:9944

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2751945

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2751946

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2760772

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2760773

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2760827

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2760828

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2761416

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2761646

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2761645

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2761772

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2761878

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2761879

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2762432

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2762689

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2762690

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2762809

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2762808

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2763352

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2764140

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2764139

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2764474

bot bench $ pallet dev pallet_contracts

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2766469

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2766468

@athei , I wonder if it's not better to stick to the hook API' style for the Migrate trait
pub trait Migrate: Codec + MaxEncodedLen + Default {
    const VERSION: u16;
    fn step(&mut self, remaining_weight: Weight) -> (IsFinished, Weight);
}

For example if you consider the DeletionQueue migration example, it's kind of tricky to implement the max_step_weight fn, unless you preserve the DeletionQueueDepth property on the config
If you pass in the remaining_weight, you can first check if you have enough weight to fetch the length of the legacy bounded vec, and then proceed to it's deletion if there is enough weight remaining to clear it and move the item to the new storage.
Not sure it's a best idea, but we could even offer both trait with a blanket implementation like this
pub trait MaxWeightMigrate: Codec + MaxEncodedLen + Default {
    const VERSION: u16;
    fn max_step_weight() -> Weight;
    fn step(&mut self) -> (IsFinished, Weight);
}

impl<T: MaxWeightMigrate> Migrate for T {
    const VERSION: u16 = T::VERSION;

    fn step(&mut self, remaining_weight: Weight) -> (IsFinished, Weight) {
        if remaining_weight.any_lt(Self::max_step_weight()) {
            return (IsFinished::No, remaining_weight);
        }

        <Self as SimpleMigrate>::step(self)
    }
}

@athei , I wonder if it's not better to stick to the hook API' style for the Migrate trait

If possible I would prefer the current design as it moves the weight pre-checking to the shared code instead of having every migration implement it themselves.

For example if you consider the DeletionQueue migration example, it's kind of tricky to implement the max_step_weight fn, unless you preserve the DeletionQueueDepth property on the config
If you pass in the remaining_weight, you can first check if you have enough weight to fetch the length of the legacy bounded vec, and then proceed to it's deletion if there is enough weight remaining to clear it and move the item to the new storage.

It does not matter whether you do the check in max_step_weight or step. In both cases you rely on the DeletionQueueDepth property as you cannot check the size of a storage item in constant time. Reading the size requires loading the whole thing from storage and hence into the PoV.
In my opinion we should require migration steps to be bounded and hence go with the current design. That is until we encounter an example which requires a change.

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2785173

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2785174

you cannot check the size of a storage item in constant time. Reading the size requires loading the whole thing from storage and hence into the PoV.

Oh right, just reading the size with decode_len save you a bit of ref_time (for not decoding the full Vec) but still account for a full read of the value and thus impact the POV

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2798025

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2798026

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2798357

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2798358

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2806558

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2807002

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2806986

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2808037

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2813302

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2813620

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2814423

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2816294

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2816293

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2816383

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2816568

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2816570

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2816569

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2817981

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2818069

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2818068

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2818067

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2818348

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2818347

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2838608

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2842720

weights => https://weights.tasty.limo/compare?unit=weight&ignore_errors=true&threshold=10&method=asymptotic&repo=substrate&old=master&new=pg%2Fmigration&path_pattern=frame%2F**%2Fsrc%2Fweights.rs

just asking based on title: could this have been made into something general and reusable?

@kianenigma, it was easier to build it inside the pallet first, but its pretty generic already and I am looking forward to making it a broader solution that can be used by other pallets, once it has been battled tested.

pls merge master into your branch

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2901405

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2901391

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2902444

bot merge