Incorrect S3 url when using Digital Ocean Spaces as staging and Clickhouse as destination

dlt version
1.1.0
Describe the problem
Using DigitalOcean S3-compatible Spaces storage as staging for loading into Clickhouse destination fails due to incorrect URL building.
Expected behavior
Data should load correctly, but instead a DB exception is thrown by Clickhouse, e.g.
2024-09-29 15:27:21,065|[ERROR]|341017|140111794464320|dlt|reference.py|run_managed:403|Transient exception in job _dlt_pipeline_state.55206d6956.reference in file /home/g/.dlt/pipelines/s3_pipeline/load/normalized/1727612187.8839986/started_jobs/_dlt_pipeline_state.55206d6956.5.reference
Traceback (most recent call last):
  File "/home/g/git/pdmdlt/.venv/lib/python3.10/site-packages/clickhouse_driver/dbapi/cursor.py", line 111, in execute
    response = execute(
  File "/home/g/git/pdmdlt/.venv/lib/python3.10/site-packages/clickhouse_driver/client.py", line 382, in execute
    rv = self.process_ordinary_query(
  File "/home/g/git/pdmdlt/.venv/lib/python3.10/site-packages/clickhouse_driver/client.py", line 580, in process_ordinary_query
    return self.receive_result(with_column_types=with_column_types,
  File "/home/g/git/pdmdlt/.venv/lib/python3.10/site-packages/clickhouse_driver/client.py", line 212, in receive_result
    return result.get_result()
  File "/home/g/git/pdmdlt/.venv/lib/python3.10/site-packages/clickhouse_driver/result.py", line 50, in get_result
    for packet in self.packet_generator:
  File "/home/g/git/pdmdlt/.venv/lib/python3.10/site-packages/clickhouse_driver/client.py", line 228, in packet_generator
    packet = self.receive_packet()
  File "/home/g/git/pdmdlt/.venv/lib/python3.10/site-packages/clickhouse_driver/client.py", line 245, in receive_packet
    raise packet.exception
clickhouse_driver.errors.ServerException: Code: 499.
DB::Exception: Failed to get object info: No response body.. HTTP response code: 302: while reading _dlt_pipeline_state/1727612187.8839986.55206d6956.jsonl: While executing S3Source. Stack trace:

0. DB::Exception::Exception(DB::Exception::MessageMasked&&, int, bool) @ 0x000000000d0fa5bb
1. DB::S3Exception::S3Exception<String const&, unsigned long>(Aws::S3::S3Errors, fmt::v9::basic_format_string<char, fmt::v9::type_identity<String const&>::type, fmt::v9::type_identity<unsigned long>::type>, String const&, unsigned long&&) @ 0x00000000103ec34a
2. DB::S3::getObjectInfo(DB::S3::Client const&, String const&, String const&, String const&, bool, bool) @ 0x00000000103ec6a8
3. DB::S3ObjectStorage::getObjectMetadata(String const&) const @ 0x00000000103e2cfe
4. DB::StorageObjectStorageSource::KeysIterator::nextImpl(unsigned long) @ 0x0000000010337a05
5. DB::StorageObjectStorageSource::IIterator::next(unsigned long) @ 0x0000000010334ef9
6. DB::StorageObjectStorageSource::createReader(unsigned long, std::shared_ptr<DB::StorageObjectStorageSource::IIterator> const&, std::shared_ptr<DB::StorageObjectStorage::Configuration> const&, std::shared_ptr<DB::IObjectStorage> const&, DB::ReadFromFormatInfo const&, std::optional<DB::FormatSettings> const&, std::shared_ptr<DB::KeyCondition const> const&, std::shared_ptr<DB::Context const> const&, DB::SchemaCache*, std::shared_ptr<Poco::Logger> const&, unsigned long, unsigned long, bool) @ 0x00000000103329ec
7. DB::StorageObjectStorageSource::createReader() @ 0x0000000010331c87
8. DB::StorageObjectStorageSource::generate() @ 0x0000000010331e96
9. DB::ISource::tryGenerate() @ 0x0000000012fbba35
10. DB::ISource::work() @ 0x0000000012fbb4c2
11. DB::ExecutionThreadContext::executeTask() @ 0x0000000012fd5347
12. DB::PipelineExecutor::executeStepImpl(unsigned long, std::atomic<bool>*) @ 0x0000000012fc9c30
13. void std::__function::__policy_invoker<void ()>::__call_impl<std::__function::__default_alloc_func<DB::PipelineExecutor::spawnThreads()::$_0, void ()>>(std::__function::__policy_storage const*) @ 0x0000000012fcb2ae
14. ThreadPoolImpl<ThreadFromGlobalPoolImpl<false, true>>::worker(std::__list_iterator<ThreadFromGlobalPoolImpl<false, true>, void*>) @ 0x000000000d1b345b
15. void std::__function::__policy_invoker<void ()>::__call_impl<std::__function::__default_alloc_func<ThreadFromGlobalPoolImpl<false, true>::ThreadFromGlobalPoolImpl<void ThreadPoolImpl<ThreadFromGlobalPoolImpl<false, true>>::scheduleImpl<void>(std::function<void ()>, Priority, std::optional<unsigned long>, bool)::'lambda0'()>(void&&)::'lambda'(), void ()>>(std::__function::__policy_storage const*) @ 0x000000000d1b74b1
16. void* std::__thread_proxy[abi:v15000]<std::tuple<std::unique_ptr<std::__thread_struct, std::default_delete<std::__thread_struct>>, void ThreadPoolImpl<std::thread>::scheduleImpl<void>(std::function<void ()>, Priority, std::optional<unsigned long>, bool)::'lambda0'()>>(void*) @ 0x000000000d1b6263
17. ? @ 0x00007883062ecac3
18. ? @ 0x000078830637e850

Steps to reproduce
If bucket_url is set as "s3://bucket_name/", then Clickhouse gives error "Bucket name length is out of bounds in virtual hosted style S3 URI" because file URL is converted to "http://bucket_name.nyc3.digitaloceanspaces.com//dataset/_dlt_pipeline_state/1727612187.8839986.55206d6956.jsonl"
Setting bucket_url as "s3://bucket_name" still doesn't work, as http endpoint is called (even if endpoint is endpoint_url is set with https), which receives a 302 HTTP status that is not acted upon.
Operating system
Linux
Runtime environment
Local
Python version
3.10
dlt data source
No response
dlt destination
No response
Other deployment details
No response
Additional information
By setting use_https=True in clickhouse.py the 302 problem is fixed. I also recommend dealing with double slashes (i.e. //) that are built in URLs.

@gfrmin before I start looking at this: I don't quite understand from your description wether you got it to work with the use_https? Was that the thing that you needed to do so it runs?

Two things are required:

use_https = True
to set bucket_url without a trailing slash, i.e. s3://bucket_name and not s3://bucket_name/

@gfrmin: I have made a PR where the use_https now is configurable and is set to true by default. If you like you can give it a spin to see wether it works alright. As for the bucket url, this should always be given without a trailing slash and i you see any mentions in the docs where there is a trailing slash let me know, then I will fix that too.


Add faq for github rate limit

Added workaround that can be done when gptscript execution results in github rate limit errors in FAQ section.

@njhale @drpebcak I have made changes as per your suggestions.


LoRA hashes are not using correct hash function

What happened?
LoRAs whose file format is .safetensors seem to currently be calculated by hashing the entire file. This is not how it should be done. Because there is very often metadata embedded in this format, and is done so by default, kohya-ss had implemented a function to hash only the weights (the part of the file that would remain unchanged). This is also how the webui calculates the hash.
https://github.com/kohya-ss/sd-scripts/blob/4c6f3125fce31f86aa949026030e923f3d7d826b/library/train_util.py#L1679-L1693
https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/102b6617dacffdcc89c56badcaae6c5e83c3ff21/modules/hashes.py#L66-L80
Steps to reproduce the problem

View any LoRA page. Using https://civitai.com/models/101104 as an example.
Several truncated hashes are listed.
None of these match what the webui calculates (a33ded9e396c)

What should have happened?
There should have been a hash listed that only hashed the model weights.

I used https://github.com/arenasys/stable-diffusion-webui-model-toolkit to prune 1000+ Loras stored locally. Now I can't use https://github.com/zixaphir/Stable-Diffusion-Webui-Civitai-Helper to index and update those Loras. Either Civitai should prune all Loras before calculating the hash, or do what @catboxanon suggested.


Added payment related functionalities to Commerce Module #483

Added the three mentioned functionalities in issue #483:

paymentId
paymentType
paymentProvider

You need to remove externals/faker-cxx directory


Applications autoinstall created.

Please separate the dependencies install (needs to be installed into whole Spark cluster) and application install (only on Spark Master). The main.yml for the role should look like:

- name: Install Application dependencies
  include: install_dependencies.yml
  become: yes

- name: Install Example application
  include: install applications.yml
  become: yes
  become_user: "{{ user }}"
  when: "'sparkMaster' in group_names"

So that you do not need to copy   when: "'sparkMaster' in group_names" thorough whole .yml.

Also, please remove the requirement  for example-application from  Stream4Flow/provisioning/ansible/sparkSlave.yml


Add Google Maps functionality

I'll probably merge this and change a few things:

[x] class="watermark" use the normal old Google logo instead of the Google Maps logo (class="lmygoc" will be the Maps logo)
[x] Fix favicon to be an ICO file

Maps seems to have a bunch of TrustedHTML stuff that doesn't allow for most ways of appending elements, including the innerHTML function. The following seems to be the simplest way to add a new element that actually works:
var link = document.createElement("link");
link.rel = "icon";
link.href = "https://stackoverflow.com/favicon.ico";
document.head.appendChild(link);

Changes manually added, expected to release as part of v2.2.


Update and rename getToken.test.ts to getTokenWithProxy.test.ts

Add proxy for getToken option

This should be a working test for all environments. If you want to test the behavior when passing httpsAgent, I recommend you to...

Mock proxy and underlying requests using jest.spy or similar and then check it uses the correct agent/proxy url.
Make proxy injected via process.env in case someone wants to run this test on the real world.

sure! can you double check?


debug maven based devappserver

we can run it. let's make sure we can debug it.
It didn't work when last I tried it. This may be an issue in the eclipse launch configuration supplied by the archetype.

https://dzone.com/articles/debugging-maven-build-mvndebug may be relevant

Definitely does not work out of the box. ATM, I'm stumped. If anyone else sees a way to fix this, go for it.

I've been poking at this and it seems to be due to a timeout.  We launch dev_appserver with the following command-line:

path/to/google-cloud-sdk/bin/dev_appserver.py  workspace/.metadata/.plugins/org.eclipse.wst.server.core/tmp0/example-appengine-archetype —api_host localhost —api_port 8188 —jvm_flag=-Xdebug --jvm_flag=-Xrunjdwp:transport=dt_socket,server=y,suspend=y,quiet=y,address=53299 --jvm_flag=-Dappengine.user.timezone=UTC

So we expect the JVM to be listening on port 53299 for a debug connection.  In our CloudSdkLaunchConfigurationDelegate, in our server-state listener, we wait 500ms after we think the dev_appserver should have been started and be in a ready state, and then try to connect, but it fails (log below).  I suspect the 500ms isn't quite enough.
if (event.getState() == IServer.STATE_STARTED) {
  try {
    Thread.sleep(500);
    runDebugTarget(cloudSdkServer, modules[0].getProject().getName(), port);
  } catch (InterruptedException | CoreException e) {
    Activator.logError(e);
  }
}

I see on my log (slightly shortened):
!ENTRY com.google.cloud.tools.eclipse.appengine.localserver 4 1 2016-06-14 15:07:15.236
!MESSAGE Google Cloud SDK Error
!STACK 1
org.eclipse.core.runtime.CoreException: Failed to connect to remote VM. Connection refused.
	at org.eclipse.jdt.internal.launching.SocketAttachConnector.abort(SocketAttachConnector.java:98)
	at org.eclipse.jdt.internal.launching.SocketAttachConnector.connect(SocketAttachConnector.java:155)
	at org.eclipse.jdt.internal.launching.JavaRemoteApplicationLaunchConfigurationDelegate.launch(JavaRemoteApplicationLaunchConfigurationDelegate.java:85)
	at org.eclipse.debug.internal.core.LaunchConfiguration.launch(LaunchConfiguration.java:885)
	at org.eclipse.debug.internal.core.LaunchConfiguration.launch(LaunchConfiguration.java:739)
	at org.eclipse.debug.internal.core.LaunchConfiguration.launch(LaunchConfiguration.java:731)
	at com.google.cloud.tools.eclipse.appengine.localserver.server.CloudSdkLaunchConfigurationDelegate.runDebugTarget(CloudSdkLaunchConfigurationDelegate.java:197)
	at com.google.cloud.tools.eclipse.appengine.localserver.server.CloudSdkLaunchConfigurationDelegate.access$0(CloudSdkLaunchConfigurationDelegate.java:191)
	at com.google.cloud.tools.eclipse.appengine.localserver.server.CloudSdkLaunchConfigurationDelegate$1.serverChanged(CloudSdkLaunchConfigurationDelegate.java:101)
	at org.eclipse.wst.server.core.internal.ServerNotificationManager.broadcastChange(ServerNotificationManager.java:125)
	at org.eclipse.wst.server.core.internal.Server.fireServerStateChangeEvent(Server.java:745)
	at org.eclipse.wst.server.core.internal.Server.setServerState(Server.java:653)
	at org.eclipse.wst.server.core.model.ServerBehaviourDelegate.setServerState(ServerBehaviourDelegate.java:143)
	at com.google.cloud.tools.eclipse.appengine.localserver.server.CloudSdkServerBehaviour.setServerStarted(CloudSdkServerBehaviour.java:80)
	at com.google.cloud.tools.eclipse.appengine.localserver.server.CloudSdkServerStartupChecker.ping(CloudSdkServerStartupChecker.java:107)
	at com.google.cloud.tools.eclipse.appengine.localserver.server.CloudSdkServerStartupChecker.access$0(CloudSdkServerStartupChecker.java:85)
	at com.google.cloud.tools.eclipse.appengine.localserver.server.CloudSdkServerStartupChecker$1.run(CloudSdkServerStartupChecker.java:67)
Caused by: java.net.ConnectException: Connection refused
	at java.net.PlainSocketImpl.socketConnect(Native Method)
        […]
	at org.eclipse.jdi.internal.connect.SocketTransportService$2.run(SocketTransportService.java:148)
	at java.lang.Thread.run(Thread.java:745)
!SUBENTRY 1 org.eclipse.jdt.launching 4 113 2016-06-14 15:07:15.241
!MESSAGE Failed to connect to remote VM. Connection refused.
!STACK 0
java.net.ConnectException: Connection refused
	at java.net.PlainSocketImpl.socketConnect(Native Method)
[…]
	at org.eclipse.jdi.internal.connect.SocketTransportService$2.run(SocketTransportService.java:148)
	at java.lang.Thread.run(Thread.java:745)

Rather than bump that timeout, we can instead change invert the connection and have the debugger be in socket-listen mode.  This seems to work very well.

Oops, I might have misappropriated this issue: on rereading the first comment, it looks like @elharo meant using the Debug As → DevAppServer launch configuration.  I created #247 to track that.


Real-time use

Hello Jinay,
Thank you for this nice project.
Is it possible to use this in real-time applications? If possible, how should I go about changing the code in your opinion :)) ?

Hi @SuperKogito Yes you could use the NVIDIA CUDA Accelerations by converting the Model to the TensorRT and add new inference Engine to use NVIDIA Inference APIs and Model. If this is not an option, you could use the TFLite Model (Android/iOS compatible) to use this in real-time by leveraging the Neural Engines that modern devices offer.

Thank you for this fast response <3
Did you try to convert the model using TensorRT? I took a look at the library and it has no maxpooling layer, which is as far as I know part of Spleeter architecture.
Leveraging the Neural Engines that modern devices offer is actually exactly what I am trying to do but with https://github.com/gvne/spleeterpp, Currently I am failing to use a Tensorflow built from source though. As you are more experienced with inference engines, do you think building Tensorflow from source helps accelerate the project or it is just a minor improvement?
Based on my analysis of  your neat project, I noticed that this library mainly do inference, on the wav file read and passed as a CLI argument. Is there any handling of the buffering needed for RT, e.g. Ring buffers for writing and reading that makes the library integration into a VST easier?
I am sorry for these many questions but I got one more, in your experience, what's the expected latency?
I really appreciate you taking the time to answer me. Thank you so much :))

Did you try to convert the model using TensorRT?
No, I haven't try! You are welcome to try and please provide your feedback so that others can benefit.

do you think building Tensorflow from source helps accelerate the project or it is just a minor improvement?
I don't think building TF from source will help since it can still be optimised for CPU (uses Intel Acceleration instruction sets), for NVIDIA GPU it may use CUDA. Ideally, these binaries you could download readily and use. But since they don't offer libtensorflow_cc.so (monolithic) binaries from TF, unfortunately. You have to build it manually on your system. Instructions can be found here

Is there any handling of the buffering needed for RT, e.g. Ring buffers for writing and reading that makes the library integration into a VST easier?
Yes, you have to write complete RTSP like buffer to feed the stream in real-time. Use FFMPEG and extend the existing code to support RTSP (Real-Time Streaming Protocol). Google for more info, you will also get some good GitHub Repositories for this.

what's the expected latency?
I have not estimated/calculated for this Model so can't comment really. But since the model is quite small, it should be pretty fast if you have decent accelerated platform.


Multi rootnode support

Added support for multiple RootNodes (bootstrap peers)
Original discussion was here
Description
All peers can now start with an array of root nodes instead of a single one, for example (from core.yaml):
    discovery:

        # The root nodes are used for bootstrapping purposes, and generally
        # supplied through ENV variables
        # It can be either a single host or a comma separated list of hosts.
        rootnode: <IP_ADDRESS>,<IP_ADDRESS>

This removes the SPOF of having a peer depend on a single root node for joining the cluster.
The method that supports this is GetRootNodes() // notice the plural form //
The original GetRootNode() method is not present anymore and instead is replaced by GetRandomNode() which returns a random root node.

In addition, NVP nodes now query a random validating peer out of the root node list instead of the same one each time

In terms of discovery process, validating peers now connect to all root nodes at startup, and non-validating peers seek a servicing peer to connect to.

How Has This Been Tested?
Ran 4 VPs and 1 NVP and invoked transactions through the VPs and NVP and ensured via reading the logs that NVP selects a random node each time, and VP calls itself and doesn't forward, and also ensured that membership view is full for all nodes via querying the REST endpoint /network/peers for all nodes.

Ran the unit tests (go test -timeout=20m $(go list github.com/hyperledger/fabric/... | grep -v /vendor/ | grep -v /examples/))
Behave tests ran by Travis

Checklist:

[x] I have added a Signed-off-by
[x] I have run gofmt, and goimports and have resolved all errors and warnings.
[x] I updated the comments in the core.yaml file
[x] I have added tests to cover my changes.

Signed-off-by: Yacov Manevich<EMAIL_ADDRESS>

I wish you could re-open a closed PR

Based on commends from #1482 , this is good to go.


CA Certs verified under BRs <IP_ADDRESS> use <IP_ADDRESS>.2

BRs <IP_ADDRESS> states that OV certificates (with policy identifer <IP_ADDRESS>.2.2) must include the localityName to the extent such a field is required under Section <IP_ADDRESS>.2.
Section <IP_ADDRESS>.2 is only applicable to Subscriber Certificates. zlint is currently enforcing this check on CA certificates.
Example: https://censys.io/certificates/95c074e35902a14abd9d19afb6e7f80e669ff8e2363270539d963613f04aaa21/zlint
It also looks like problems exists with <IP_ADDRESS>.2.3 (EV) certificates as well. I have only analyzed code related to <IP_ADDRESS>.
Potential fix:
It seems like it would be as simple as adding a cert.IsCA check to the affected code.
https://github.com/zmap/zlint/blob/5899dfa3116b1f4c9f88e6a4dab18f72e5836812/lints/lint_cert_policy_ov_requires_province_or_locality.go#L36
https://github.com/zmap/zlint/blob/5899dfa3116b1f4c9f88e6a4dab18f72e5836812/lints/lint_cert_policy_iv_requires_province_or_locality.go#L36
Reference:
https://cabforum.org/wp-content/uploads/CA-Browser-Forum-BR-1.5.6.pdf

Updated the relevant tests to only apply to subscriber certificates in #207.

I've merged #207.

Thanks for the prompt fix!


Fixes # (issue no.)
#133
Type of change

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] This change requires a documentation update

Explain the Testing instructions
Test Configuration:

Operating system: windows
Version: 11
Text-editors used: vscode

Checklist:

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[ ] My changes generate no new warnings
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] New and existing unit tests pass locally with my changes
[ ] Any dependent changes have been merged and published in downstream modules

ATTACH SCREEN-SHOTS / DEPLOYMENT LINK

please merarge with hacktoberfest-accepted comment


Ensure we can write tests for generated client

We can't currently write integration tests for the generated client because the client is generated (but not compiled) at the point at which the integration tests are compiled.
Definition of done

We can write an integration test which tests the generated client against a real Cassandra server.

Marking as urgent because we want to merge the integration branches, and we've decided we definitely need integration tests before doing so.

After discussing this with Erik, it seems like this is unlikely to happen.
I recommend to take this out of the pre-req for merging the integration branch and revisit how we can handle this because not having tests makes me as nervous as you.


Enforce max frame size

#2651 The MaxFrameSize setting wasn't being enforced so instead you'd get an ArgumentOutOfRangeException for frames larger than the buffer. The h2spec test 4.2 found this, but it wasn't actually failing because the exception was still handled and terminated the connection, only with the wrong http2 error.

Correct, those failures are unrelated infrastructure issues.


Mock matrix at the client level

We are working on changing how matrix gets mocked in tests. Right now the whole matrix service gets mocked, which means we're unable to exercise a large part of our actual logic. This also forces the implementation of the matrix service to be splatted over several files.
Instead, we want to mock the actual Matrix client that underlies the matrix service and let the matrix service function in tests.

Next action: we see test waiting problems since the matrix service's queues are debounced in a non-test-friendly way.


NotImplementedException when using @RequestScope()

I'm getting compilation error when using @RequestScope() decorator.
Is there a missing part I should implement to use this ?
node_modules/nj-request-scope/lib/util/dynamic.object.handler.factory.js:27
            throw new NotImplementedException();
                  ^
Error: Not implemented. Feel free to implement.
    at Object.getPrototypeOf (//node_modules/nj-request-scope/lib/util/dynamic.object.handler.factory.js:27:19)
    at Function.[Symbol.hasInstance] (<anonymous>)
    at mapToProxy (node_modules/@nestjs/core/nest-factory.js:167:35)
    at Proxy.<anonymous> (//node_modules/@nestjs/core/nest-factory.js:182:32)
    at bootstrap (/src/main.ts:39:16)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

Hi @AltarBeastiful,
Initially, I implemented only those Proxy methods I needed. The rest of them are throwing a "Not implemented. Feel free to implement" error.
I've prepared a new version of nj-request-scope with 'getPrototypeOf' implementation, which you need.
Please, try the new release candidate version:  1.0.4-rc.0 and let me know if all is working properly.

Indeed it works !
Will try to contribute if I'm stuck again on one.

It's great to hear :) Thank you for testing!
I've released a final version with this fix: 1.0.4.


Revert broken commit

Reverts commit 49035f4d4142b3a7f0a53f7dd966437d1f6b2c43, which broke the argon tests. I'm not sure what exactly about these changes caused the build to break, but I've confirmed that the argon tests pass with these changes reverted.

I submitted #363 to fix an overflow issue where a sum of u8 values could exceed the u8 range.


Address Owen's feedback on Web Bluetooth Origin Trial

FIX: https://github.com/google/WebFundamentals/pull/3147#issuecomment-237013680
TBR: @petele

@owencm This merged PR addresses your feedback. Thanks!
@petele Can you push this live?

Thanks for addressing, this is really helpful!
Sorry I just noticed it, but can we make one last change for the 'origin trials' text to link to https://github.com/jpchase/OriginTrials/blob/gh-pages/developer-guide.md rather than the repo itself?


[$250] [HOLD for payment 2024-12-09] [Per Diem] Frontend - Importing rates for Per Diem in Newdot

Part of the Add Per Diem to New Expensify project
Main issue: https://github.com/Expensify/Expensify/issues/400276
Doc section: https://docs.google.com/document/d/1c6WDOJlhvODD0mRKaCHfQk_HRNmq5K7hSDHLu06Pbq8/edit?tab=t.0#bookmark=id.v57hwv1fqs85
Project: https://github.com/Expensify/Expensify/issues/400276
Feature Description
Add the necessary changes to import Per Diem rates in newdot workspaces
Manual Test Steps
Automated Tests
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021866231379227094255
Upwork Job ID:<PHONE_NUMBER>227094255
Last Price Increase: 2024-12-09

Not overdue

Contributor+: @c3024 is due $250 via NewDot.

Only @c3024 requires payment here. I will paid at the end of the project.

d'oh, saw this after inviting you to an Upwork job @shubham1206agra , plz ignore it.
I'm assuming that test cases will be created as part of the larger project, comment or reopen if that's not correct @Gonals and @shubham1206agra

$250 approved for @c3024


feat: migrate to JASSUB for ASS rendering

Description:
This PR migrates from suggesting JSSO as a fallback ASS renderer to JASSUB. Both libraries do the same thing: wrap libass WASM with some nice JS, but JASSUB does it differently.
JASSUB handles rendering a LOT differently from JSSO and as such it implements a lot of new rendering modes:

on demand render - this renders subtitles ONLY when a new video frame is about to be painted on the user's screen, this does away with the whole timestamp and event system JSSO used, and it's immensely accurate allowing for pretty much flawless rendering of any custom TS example [not that any english text u see on that video is TS, which might not be obvious at first glance], this is also of amazing benefit for very high framerate videos and variable framerate videos, as it renders together with the video frame, not at some abstract 24 fps [note the yellow square is the subtitle, the white one is the video, this showcases tracking]
offscreen render - this uses the offscreen canvas API to render EVERYTHING fully on the worker, meaning the main web app itself won't lag no matter how heavy the subtitles are
blend modes - the reason JSSO's JS render was slow was because it did decoding on JS, JASSUB does this in WASM, which means JS render will be a LOT lighter and marginally faster than WASM blend, but this only applies if the user has a dedicated GPU

JASSUB handles fonts completely differently from JSSO, which makes it a lot faster

you don't even need any fonts for the library to init, which means you don't need any data files for the library to work
it supports dynamic font loading
it can load fonts directly from memory instead of from url
it can load fonts directly from the users device if his browser supports it

JASSUB handles video resizing completely differently

videos can resize dynamically at any point, JASSUB handles that individually per frame, and updates the canvas size once a new video frame is about to render, this is in contrast to JSSO which just used timeouts which cleared the canvas and caused a lot of performance issues
videos can be anamorphic which JSSO also didn't handle, JASSUB scales the subtitles correctly on non-firefox browsers [ff doesn't expose anamorphic metadata for videos]

JASSUB is compiled differently

it uses lembind instead of webIDL, this leads to a roughly 500kb size reducement of the output WASM and JS as the bindings are dynamic
since it drops webIDL it can use newer versions of emscripten [emsdk:2.0.34 -> emsdk:3.1.24]
since a lot of old garbage was removed we can use features like minimal runtime, nomain, and compiled streaming which means the WASM itself is loaded and executed a LOT quicker, we're speaking >x2 speed

JASSUB is >2MB smaller when including the optional files, and ~1MB smaller when excluding optional files.
Ready?
Yes.
Anything Else?
Full JSSO comparison can be viewed here together with interactive examples at the bottom of the list.
I noticed that removing a text renderer doesn't call it's dispose/destroy function. Is this intentional?
Review Process:

Thanks heaps for this detailed PR! I'm working through few things right now but I'll review this as soon as I can.

since this PR was added I also added support for multiplying the subtitle color matrix based on the video and subtitle colorspaces, TLDR it means the subtitles will now have accurate colors on videos whose colorspaces don't match those of their subtitle tracks, which JSO also didn't handle


隐私声明和签名

As per the Upcoming third-party SDK requirements using OpenSSL in an App on the App Store will require a privacy manifest (cf. Privacy manifest files) starting May 1st. Since the package comes with precompiled libraries a code signature may also be required. Is that something that you would consider providing?

@luoxiankang2 my app is continuously reject from apple app store..
Please correct the following issues and upload a new binary to App Store Connect.
ITMS-91065: Missing signature - Your app includes “Frameworks/openssl.framework/openssl”, which includes BoringSSL / openssl_grpc, an SDK that was identified in the documentation as a privacy-impacting third-party SDK. If a new app includes a privacy-impacting SDK, or an app update adds a new privacy-impacting SDK, the SDK must include a signature file. Please contact the provider of the SDK that includes this file to get an updated SDK version with a signature. For details about verifying the code signature for a third-party SDK, visit: https://developer.apple.com/documentation/xcode/verifying-the-origin-of-your-xcframeworks.


flaw

Something went wrong：
jest-editor-support parser returned error SyntaxError: Decorators cannot be used to decorate parameters. (18:20)
eg:
`
@Injectable()
class TestService {
protected async findAll(): Promise<number[]> {
return [1, 2, 3, 4, 5];
}
async mutexTTLKey(@LockKey() key: string): Promise<number[]> {
return await this.findAll();
}
}
describe('lock decorator', () => {})
`
The above error will occur when @LockKey() exists
I am using nestjs

same issue, also this case fails too so that CodeLens will be broken:
@A().B()
class Test {}

same issue, also this case fails too so that CodeLens will be broken:
@A().B()
class Test {}

me too, when I use parameter decorator，
the "run | debug" button disappear

check new version which includes new version of jest-editor-support.
please re-open if this issue still persists.


Minor improvements

This branch contains several minor improvements over the board. In particular it factors out a lot of the common parts in different methods found in src/utilities.jl, but it also contains some other updates.

Codecov Report

Merging #8 (41f050f) into master (bb38864) will decrease coverage by 1.42%.
The diff coverage is 88.05%.

@@             Coverage Diff             @@
##            master       #8      +/-   ##
===========================================
- Coverage   100.00%   98.57%   -1.43%
===========================================
  Files            6        7       +1
  Lines          570      560      -10
===========================================
- Hits           570      552      -18
- Misses           0        8       +8

Flag
Coverage Δ

unittests
98.57% <88.05%> (-1.43%)
:arrow_down:

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

Impacted Files
Coverage Δ

src/extrema_enclosure.jl
96.95% <81.25%> (-3.05%)
:arrow_down:

src/utilities.jl
88.88% <88.88%> (ø)

src/extrema_polynomial.jl
100.00% <100.00%> (ø)

src/extrema_series.jl
100.00% <100.00%> (ø)

src/isolate_roots.jl
100.00% <100.00%> (ø)

src/refine_root.jl
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 bb38864...41f050f. Read the comment docs.


Feature contact support

Added a menu item that fires an email intent.

reopened branch as I plan to add unit test still


Classic PBFT hangs under stress

Observed in
commit 7abd041f5b9e5ce346d1608f26b6880f6e235a77
Merge: 5077256 4fd8ae5
Author: Binh Q Nguyen<EMAIL_ADDRESS>Date:   Wed Feb 17 13:29:30 2016 -0500

    Merge pull request #682 from adecaro/crash_issue669

    Fix for issue #669

Same setup and irritation as in #687. I have a 4-peer docker-compose network running classic PBFT using all default parameters. I send 1000 transactions as fast as possible. The good news is that the network usually does not crash (#696 is rare) and eventually arrives at the correct answer. The issue is that it takes 8 or 9 minutes after I send in the last transaction for the database to correctly record the last transaction.
You can see in this trace that the network is only processing 20 or 40 transactions every 30 seconds after the last transaction has been sent over the REST API. Note that security is not enabled in this network (#684).
[bcbrock@arlab178]$ driveEx02 <IP_ADDRESS> -rest -n 1000 -wait 30 -reCheck
Waiting 30 seconds for chain state to stabilize
 A: Got -638, expected -1000 B: Got 638, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -658, expected -1000 B: Got 658, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -678, expected -1000 B: Got 678, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -698, expected -1000 B: Got 698, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -738, expected -1000 B: Got 738, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -758, expected -1000 B: Got 758, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -778, expected -1000 B: Got 778, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -798, expected -1000 B: Got 798, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -838, expected -1000 B: Got 838, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -858, expected -1000 B: Got 858, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -878, expected -1000 B: Got 878, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -898, expected -1000 B: Got 898, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -938, expected -1000 B: Got 938, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -958, expected -1000 B: Got 958, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -978, expected -1000 B: Got 978, expected 1000
Waiting 30 seconds for chain state to stabilize
 A: Got -998, expected -1000 B: Got 998, expected 1000
Waiting 30 seconds for chain state to stabilize
Runtime: 4.519867 seconds
Rate   : 221.2 transactions/second

Note 221 TX/sec is only for the initial send of 1000 TX.
Looking at top, the peers/chaincode are practically idle during this 8-9 minute period. Thinking this might be due to I/O I took a dstat visualization; I/O is nominal during this period.

Here are the full DEBUG -level logs from the 4 peers.
logs.tgz.zip

This is likely a combination of multiple bugs.
One is that because of null request pruning, the new primary does not include any requests in the xset, and therefore does not update its own seqNo.  This prevents the new primary to assign sequence numbers to the outstanding requests, which in turn renders it useless.  Once N=4 view changes have occurred, vp0 becomes primary again and manages to pick up where it left off.

The other bug is that moveWatermarks needs to trigger processing of outstanding requests that have queued up, because we ran out of sequence numbers.  We should revisit all functions that use the UPON term in the PBFT paper.

while this is being fixed - it might be wise to drop "eventual" from the name of the issue

@bcbrock , please verify.

It is difficult to know if this issue has been resolved or not, since the OBC protocol documentation does not address what is supposed to happen when a network is flooded with more legitimate transactions than it can process in real time.
In my latest test I used 64 clients to each send 1000 transactions as fast as possible. The 64K transactions were all accepted by the network in 216 seconds. However, 30 seconds later we see that only 1500 have actually been processed. The network then proceeds to process the backlog at an average rate of about 5 TX/seconds. It would have taken this system about 3.5 hours to process the backlog if it had not failed. This is much faster than before, where we saw the backlog processed at about 1 TX/second.
After 10 minutes working the backlog the network stopped processing transactions. As far as I can tell, none of the peers or chaincodes crashed, they simply appear to have stopped working on the backlog. I don't see any obvious "smoking gun" error message in the logs, but peer 1 does report some errors that I have never seen before:
/home/bcbrock/vp1_logs/stderr:14:40:56.231 [consensus/statetransfer] stateThread -> ERRO 18de65 name:"vp1"  could not retrieve blocks as recent as 4360 as the block hash advertised
/home/bcbrock/vp1_logs/stderr:14:40:56.233 [consensus/statetransfer] stateThread -> ERRO 18de89 name:"vp1"  believed its state for block 4360 to be valid, but its hash (58c3abaf01657c5a08e2c303d719db683a87a329898cf84d4e72c08f6d665f3d892e47cd6ad5a0b2b37023f6e1be4665ed5f6881f01997f5d5433c6d1e6202bf) did not match the recovered blockchain's (5b6900f1ce29b19d3d2d00350b0ff9f6e6707a3f2b9f49b7044f8acd6637161a38cd5e4add7707074d179b3342966be21ae6c2bc17154258eb780f49544c06f4)
/home/bcbrock/vp1_logs/stderr:14:40:56.620 [consensus/statetransfer] stateThread -> ERRO 192241 name:"vp1"  recovered to an incorrect state at block number 4701, (62e5208d8efd9fbb2a574d77e7bff5fef464975008f671cd33d9a0a77cf05b17a400212be5349f65da2678cef3aa96680225a6443db4578f2e50e7f0582beb54 47ccc4cf8d69ac32e69284d359fa34f908e20d013d9973d5b2138366fff00db6327a626f83dc194ff05bd9fbb2241bdf66229ade2eb5d9ad61f354ce3c450e39) retrying

Following this there are 100's of messages like the last one with increasing block numbers.

@bcbrock I'll investigate those stateThread errors, that pattern of log messages indicates a bug.

The merge request referenced above should fix some of the behavior you are observing in the logs.  It may not fix the originating issue, and but may allow recovery to complete whereas the recovery was stalled indefinitely before.

The system is still failing/locking up. It appears to be less stable than before. Previously it would fail while trying to clear up the backlog. Now it appears to fail/hang during processing of the original burst of transactions.  I have attached the ERRO/WARN messages from the logs.
errors.txt

I tried @corecode 's closed-loop-rest patch with a timeout of 60. For small numbers (1 - 4) of concurrent clients it seems to works. With 8 or 16 it fails. The failures now are all seen as "hangs" - processing stops. Here are the error messages from a 16-client run that failed almost immediately.
errors.txt

@bmos299 - This issue should be marked as a bug

This code path is being overhauled via issue #751, that work will hopefully resolve this issue.

@bcbrock @jyellick . Thanks Jason.  Added sev2 to the bug as well.

verify when #751 is merged

@bcbrock.  Are you going to merge this?  Can you coordinate with @RicHernandez2 on this?  We want to ensure thorough testing on this one.
Thanks.

@bmos299 - Not sure what you mean. I am not aware that a fix is available.

Bishop, now that PR #833 is done, can you re-run ?

OK, I am now at
commit f08ccce33b13f23eb1af97a43bf654aa48129794
Merge: ab0e167 bfecc0f
Author: tuand27613<EMAIL_ADDRESS>Date:   Thu Mar 24 17:30:24 2016 -0400

    Merge pull request #833 from jyellick/executor-split

    obcpbft Executor split

I'm running a network of 4 validating peers with multiple chain codes, sending transactions to peers randomly.  The test failed when a couple of the transactions never appeared in the blockchain. I've attached a summary of all of the errors. This one looks like  a smoking gun - everything seems to have stopped working after this.:
/home/bcbrock/vp1_logs/stderr:13:45:27.099 [consensus/obcpbft] Checkpoint -> ERRO 19b1 Attempted to checkpoint a sequence number (44) which is not a multiple of the checkpoint interval (10)

errors.txt

I reran the same test, here are the errors from that run
/home/bcbrock/vp1_logs/stderr:14:04:02.516 [peer] handleChat -> ERRO 061 Error handling message: Peer FSM failed while handling message (DISC_HELLO): current state: created, error: transition canceled with error: Error registering Handler: Duplicate Handler error: {name:"vp3"  <IP_ADDRESS>:30303 VALIDATOR }
/home/bcbrock/vp1_logs/stderr:14:06:43.856 [consensus/obcpbft] Checkpoint -> ERRO 16aa6 Attempted to checkpoint a sequence number (564) which is not a multiple of the checkpoint interval (10)
/home/bcbrock/vp1_logs/stderr:14:06:47.915 [consensus/statetransfer] stateThread -> ERRO 1a6ec name:"vp1"  could not retrieve blocks as recent as 780 as the block hash advertised
/home/bcbrock/vp1_logs/stderr:14:06:47.917 [consensus/statetransfer] stateThread -> ERRO 1a702 name:"vp1"  believed its state for block 780 to be valid, but its hash (b069036525d965e352e84c3f8435b853ed7615c768480e05c3f51e9ddbf7cef191c95583e7a47d3c32cc889a20b0ff3151213c17166b31d20a442a853d076a4c) did not match the recovered blockchain's (b265fc5744deb0ea0ae90bcb4aa96b31fad9b85646be1be7304d784863427bfbb08c0b6818ea01f0db5c1fccde87ae95b09e5d4f8cf964fe355a937438efdfc4)
/home/bcbrock/vp2_logs/stderr:14:04:02.920 [peer] handleChat -> ERRO 060 Error handling message: Peer FSM failed while handling message (DISC_HELLO): current state: created, error: transition canceled with error: Error registering Handler: Duplicate Handler error: {name:"vp3"  <IP_ADDRESS>:30303 VALIDATOR }
/home/bcbrock/vp3_logs/stderr:14:04:02.517 [peer] handleChat -> ERRO 05e Error handling message: Peer FSM failed while handling message (DISC_HELLO): current state: created, error: transition canceled with error: Error registering Handler: Duplicate Handler error: {name:"vp1"  <IP_ADDRESS>:30303 VALIDATOR }
/home/bcbrock/vp3_logs/stderr:14:04:02.920 [peer] handleChat -> ERRO 069 Error handling message: Peer FSM failed while handling message (DISC_HELLO): current state: created, error: transition canceled with error: Error registering Handler: Duplicate Handler error: {name:"vp2"  <IP_ADDRESS>:30303 VALIDATOR }

When I get back next week I'll post my test suite on a Github so you guys can run it yourselves.

The Checkpoint and stateThread errors are interesting (although perhaps they should be warnings, as they are non-fatal, and based on the lack of other errors, the recovery mechanisms seem to be working), I'll take a look at the code to see how those might arise.
I have frequently seen the handleChat errors as well, I suspect that these arise when there is a disruption of network connectivity between peers, and on renegotiation the peers have already been discovered, maybe @jeffgarratt can weigh in on those.

@jyellick there is a modification that needs to be made wrt to chat termination.  I can take a look at this shortly.

@bcbrock Can you confirm whether you are still seeing the original slow transaction rate?

@jyellick Unfortunately the system is no longer able to process the type of test that led to the original performance report without experiencing fatal errors. Yes this is "classic" PBFT.

@jyellick any progress on this one?

@binhn @jyellick I can close this. This particular issue is no longer observed.


Add tol function

Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.76%. Comparing base (7faabda) to head (2194e1e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
+ Coverage   94.64%   94.76%   +0.12%
==========================================
  Files          22       22
  Lines         989      993       +4
==========================================
+ Hits          936      941       +5
+ Misses         53       52       -1

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


[BUG]: Remove pnpm explicit version when pnpm bug is solved

Environment affected
CI/CD
Description
This bug https://github.com/pnpm/pnpm/issues/6991 forced us to select the 8.6.0 version of pnpm in the GitHub Actions in this PR: https://github.com/empathyco/x/pull/1268
We should remove the explicit version and go back to version 8 when the bug is solved.
Current behavior and expected result
CICD is not failing at install step
Steps to reproduce
Try to pass CICD pipeline in Github
Package affected
All of them
Supporting information
No response
Code of Conduct

[X] I agree to follow this project's Code of Conduct

https://github.com/empathyco/x/pull/1273


Add bottom bar

Closes #214
edit: the position of the tag display has changed so ignore that here
Dark Mode

Light mode

Codecov Report

Merging #213 into master will decrease coverage by 0.60%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master     #213      +/-   ##
============================================
- Coverage     63.96%   63.35%   -0.61%
  Complexity      893      893
============================================
  Files           143      144       +1
  Lines          3141     3171      +30
  Branches        351      351
============================================
  Hits           2009     2009
- Misses         1011     1041      +30
  Partials        121      121

Impacted Files
Coverage Δ
Complexity Δ

...c/main/java/seedu/momentum/logic/LogicManager.java
45.00% <0.00%> (-3.65%)
3.00 <0.00> (ø)

src/main/java/seedu/momentum/model/Model.java
100.00% <ø> (ø)
1.00 <0.00> (ø)

...c/main/java/seedu/momentum/model/ModelManager.java
61.26% <0.00%> (-0.28%)
55.00 <0.00> (ø)

src/main/java/seedu/momentum/ui/BottomBar.java
0.00% <0.00%> (ø)
0.00 <0.00> (?)

src/main/java/seedu/momentum/ui/MainWindow.java
0.00% <0.00%> (ø)
0.00 <0.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 f84fe99...a9e4e70. Read the comment docs.


"Error: SharedArrayBuffer is not defined" while trying to play back images

Hello,
when I try to play back the compressed image topic of my rosbag I just get the error message
Error: SharedArrayBuffer is not defined
It also says "failed to decode image on /detection_image/compressed:"

I am using Chrome version 92.0.4515.107 (Official Build) (64-bit).
I was looking on the web for the problem and it seems that google did some stuff with the SharedArrayBuffer, but I'm not familiar with the whole topic: https://developer.chrome.com/blog/enabling-shared-array-buffer/
Any idea how this could be fixed?
Thanks and BR
Kevin

@KevS272 I fixed the issue making the following changes:

packages/webviz-core/src/util/binaryObjects/binaryTranslation/index.js:178
replaced const BufferType = self.crossOriginIsolated ? SharedArrayBuffer : ArrayBuffer;  with const BufferType = ArrayBuffer;
packages/webviz-core/src/panels/ImageView/renderImage.js:121
replaced const notSharedData = rawData.buffer instanceof SharedArrayBuffer ? new Uint8Array(rawData) : rawData; with const notSharedData = rawData;
packages/webviz-core/.eslintrc.js:61
replaced SharedArrayBuffer: true, with SharedArrayBuffer: false,

unclear if all three changes are needed
unclear what the implications of changes are
maybe someone more knowledgeable can comment on this

Hello,
when I try to play back the compressed image topic of my rosbag I just get the error message
Error: SharedArrayBuffer is not defined
It also says "failed to decode image on /detection_image/compressed:"

I am using Chrome version 92.0.4515.107 (Official Build) (64-bit).
I was looking on the web for the problem and it seems that google did some stuff with the SharedArrayBuffer, but I'm not familiar with the whole topic: https://developer.chrome.com/blog/enabling-shared-array-buffer/
Any idea how this could be fixed?
Thanks and BR
Kevin

Upvoting.
I am using the web-version and facing the same issue https://webviz.io/app/

I also have the same problem. The image can be correctly encode, but compressedImg can't.

For anyone experiencing this issue, I'd encourage you to try out Foxglove Studio, which renders compressed and uncompressed images without using SharedArrayBuffers.
Foxglove Studio is a visualization tool originally based on webviz, available as a cross-platform native app and also a web app. If you have any questions or issues with it, you can contact us on GitHub or in our Slack workspace!

For anyone experiencing this issue, I'd encourage you to try out Foxglove Studio, which renders compressed and uncompressed images without using SharedArrayBuffers.
Foxglove Studio is a visualization tool originally based on webviz, available as a cross-platform native app and also a web app. If you have any questions or issues with it, you can contact us on GitHub or in our Slack workspace!

I have tried Foxglove Studio, but the plot in Studio doesn't work well as in the webviz. See in https://github.com/foxglove/studio/issues/1641.

When I tried the newest version of Webviz, I found this problem has been fixed.👍


Fix: cancel changeDate when mouseup element

fix: cancel changeDate when mouseup element
for issue  https://github.com/idrawjs/idraw/issues/159

Codecov Report

Merging #160 (2851c33) into main (0c46877) will decrease coverage by 0.04%.
The diff coverage is 33.33%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more
@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
- Coverage   27.75%   27.71%   -0.04%
==========================================
  Files          75       75
  Lines        2839     2843       +4
  Branches      611      612       +1
==========================================
  Hits          788      788
- Misses       1532     1535       +3
- Partials      519      520       +1

Flag
Coverage Δ

unittests
27.71% <33.33%> (-0.04%)
:arrow_down:

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

Impacted Files
Coverage Δ

packages/board/src/lib/watcher.ts
0.00% <0.00%> (ø)

packages/core/src/lib/engine-temp.ts
80.00% <ø> (ø)

packages/core/src/lib/engine.ts
13.14% <0.00%> (-0.31%)
:arrow_down:

packages/board/src/lib/screen-watcher.ts
21.53% <52.38%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.


Update ByoHost Validating webhook

What this PR does / why we need it:
This PR updates the existing ByoHost validating webhook for authorization rules. Unit tests are also added along with integration tests.
Authz rules

Update op will be allowed for the ByoHost owner agent user or controller manager service account.
Create op will be allowed for the resource owner agent user
Reads/Deletes are allowed to any user.

Which issue(s) this PR fixes :
Fixes #514

LGTM


chore(internet_port): added new ports and removed unnecessary string class

SUMMARY
Added new ports and removed unnecessary string class
https://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml?&page=6

Thanks @anirudh-hegde for the change. @cccs-tom would you mind taking a look at this?

@anirudh-hegde please check the linting errors:

@anirudh-hegde there's still that trailing whitespace issue:
superset/advanced_data_type/plugins/internet_port.py:93:0: C0303: Trailing whitespace (trailing-whitespace)

It means in line 91 - 93 there is a trailing whitespace?

I used pylint internet_port.py

Ohh my bad!! I merged branch 'apache:master' into spear

Should I need to change the code?

Looks like you just need to run the pre-commit hook and you should be OK.

Looks like you just need to run the pre-commit hook and you should be OK.

You were right.
Thank you, @rusackas


Need to Access the http:RequestContext within WebSocket Upgrade Service

Description:
To add the GraphQL Context support for subscription operations, the graphql:Context object is needed to be initialized per each request. Since the GraphQL module uses WebSocket as the underline protocol of subscription operations, the context object initialization is needed to be done within the WebSocket upgrade service. To initialized the context object, http:RequestContext and http:Request are required. Currently, http:RequestContext cannot be accessed within the WebSocket upgrade service.

WebSocket does not have a concept of interceptors(at least for now) for which HTTP uses this RequestContext. We can provide it as a parameter for the initial upgrade request if we can get an API from the HTTP package. But need to think about whether this is necessary from the WebSocket point of view. From what I understood interceptors are important when you have multiple resources in your service(to execute common logic across all the resources) and when you need to modify requests/responses. These are not common things in WebSocket as we don't even let the users create requests/responses. They are created internally as those should be sent in a specific format. And also there is only a single resource in the WebSocket upgrade service.
So we need to discuss before adding this.

Had a discussion about this with Thisaru, Shafreen, and Dimuthu. The final decision was to continue with setting the RequestContext to null and continue.


Import File - Uncaught Exception: java.lang.NoClassDefFoundError

Context: I tried to import the eboot.bin file of Patapon 3 EUR.
The version is "Allegrex-Ghidra-10.1.5" from the latest release.

Ghidra version 10.2.2

There's your problem, there's no 10.2.X version yet, try with 10.1.5 which is the latest Ghidra version that the extension supports

Yes, you need to use same Ghidra version as in the extension file name.
This is covered by #20.


Adding docker pull policy Parameter

Description
Adding additional parameter to specify the Docker Pull Policy for the Gitlab Runner
Migrations required
NO
Verification
has been in production in our setup for a few weeks with parameter if-not-present. default value is the default from Gitlab
Documentation
Documentation was updated to include the additional parameter. Also it was updated from upstream and fixed minor autodoc issue.

@roock thanks, look good will check the change in detail the next days.


Conditional content for radios and checkboxes

Description
Added functionality for conditionally showing content when radio buttons or checkboxes are checked.
This has been asked for in this issue: https://github.com/nhsuk/nhsuk-service-manual-backlog/issues/173
Functionality matches the following Gov examples:

https://design-system.service.gov.uk/components/radios/#conditionally-revealing-content
https://design-system.service.gov.uk/components/checkboxes/#conditionally-revealing-content

Tests written... accessibility tested on OSX and iOS... behaviour matches that of Gov. Voiceover doesn't acknowledge aria expanded change but I don't think that is possible on checkboxes. Worth somebody else doing some testing on.
JavaScript has Jest tests written.
Checklist

[x] Tested against our testing policy (Resolution, Browser & Accessibility)
[x] Follows our coding standards and style guide
[x] CHANGELOG entry

@AdamChrimes @mcheung-nhs I have got this all working as a few people have been asking for it... do you have any additional suggestions for the implementation?

Had a look on Voiceover on iPhone and it seems to make sense to me in that it says the radio button is in a collapsed state, suggesting that if you select it then it will be expanded with further information. See video here:
https://mcheung-nhs.github.io/accessibility/tests/nhs.uk/radio-conditional.mov
This is the same behaviour as that of gov's version.

This all looks pretty good to me! Just need to do some browser testing on IE etc but I think it will be OK with the JS polyfills.

Created a placeholder in the service manual guidance for these: https://github.com/nhsuk/nhsuk-service-manual/pull/609/files

I've tried the compiled JavasScript (v3.1.0) in the prototype kit and both conditional reveals are working 👍


Fix Collection::implode behaviour.

Pull Request description:
Fix issue #53.
php > $foo = new \CharlotteDunois\Yasmin\Utils\Collection([['k' => 'a'], ['k' => 'b'], ['k' => 'c']]);
php > echo $foo->implode('k', ', '), "\n";
a, b, c
php >

php > $foo = new \CharlotteDunois\Yasmin\Utils\Collection(['a', 'b', 'c,,,,']);
php > echo $foo->implode('|'), "\n";
a|b|c,,,,
php >

Why should this Pull Request be merged:
Fixes issue #53
Semantic Versioning Classification:

[ ] This PR includes major breaking changes (such as method changes)
[ ] This PR includes only documentational changes (such as docblocks, README, etc.)

Fair enough, I assumed non-mixed collections and that you wanted to keep
the old behaviour of single-argument imploding.
The committed fix works for me!
On Wed, 20 Jun 2018, 04:08 Charlotte Dunois<EMAIL_ADDRESS>wrote:

@CharlotteDunois commented on this pull request.
In src/Utils/Collection.php
https://github.com/CharlotteDunois/Yasmin/pull/54#discussion_r196636705:

@@ -409,11 +409,12 @@ function implode($col, string $glue = ', ') {
} elseif(\is_object($val)) {
$data .= $glue.$val->$col;
} else {

           $glue = $col;

But you can have a combination of entries and overwriting $glue breaks
the behaviour in case of mixed collections. So my priority is to keep (or
rather get) the expected behaviour.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/CharlotteDunois/Yasmin/pull/54#discussion_r196636705,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AALlGws8zxDLv05Dzref8SLIxLDQ1tZoks5t-byfgaJpZM4UuIvn
.


[SHIRO-640] queryForAuthenticationInfo(): resolve DN using user name

I am trying to use ActiveDirectoryRealm with searchFilter in order to make it possible to log in using e-mail (which is not part of DN). I see that this is partially supported in getRoleNamesForUser(), but not in queryForAuthenticationInfo().
This change make it fully work for me, but I have a feeling that it may disturb other users, so I'm willing to work on improving it.

We may want to combine some of the functionality in the DefaultLdapRealm with the AD realm.  Take a look at: https://github.com/apache/shiro/blob/8acc82ab4775b3af546e3bbde928f299be62dc23/core/src/main/java/org/apache/shiro/realm/ldap/DefaultLdapRealm.java#L227

Why are DefaultLdapRealm and AD realm are separated altogether?
Wouldn't it make sense to merge them?

IIRC, ActiveDirectory typically uses one or two different schemas,
depending on how it is configured.  The schema for an LDAP server could be
just about anything (but typically follow a couple patterns).
We could make them share a lot more code through the AbstractLdapRealm
On Sat, Nov 25, 2017 at 6:25 PM, mephi42<EMAIL_ADDRESS>wrote:

Why are DefaultLdapRealm and AD realm are separated altogether?
Wouldn't it make sense to merge them?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/apache/shiro/pull/74#issuecomment-346972888, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGGcnQM9ASnfsVDkSQKhAPi_JPG7bCsks5s6KHQgaJpZM4QjCnM
.

In this update I made two improvements:

shared user DN substitution logic between AbstractLdapRealm and DefaultLdapRealm
took care of potential compatibility issue, when callers take care of resolution themselves, by providing a new userSearchFilter property

Thanks @rubenvanwanzeele: #81

@bdemers What's the status of this? Is there any update on this functionality?


Try KaTeX

https://github.com/Khan/KaTeX

Still have to import resources (CSS and fonts). Not really worth it if there's still a third party link.


Multiple consistency improvements

Goal of this PR
This PR does the following:

Makes all errors consistently use could not instead of sometimes unable to
Orders API endpoints alphabetically in code and docs
Updates the DPS API Documentation which was updated
Uses the simple present tense instead of past/future tense and reduce usage of we, our, us, etc.
Makes inline comments capitalized and add dot at the end of their sentences when missing
Makes all tests use name instead of desc and description
Makes all tests be called tests instead of vectors
Improves consistency in command line argument descriptions
Fixes the test for converting Cadence values where we actually wrote Uint instead of UInt which ended up testing the case of an unknown type instead of an invalid UInt value. Also adds a test for the unknown type since it makes sense.
Removes outdated TODOs for closed issues
Adds comments to Rosetta API endpoints/response/request types with links to the relevant documentation
Makes test []byte values consistently use hexadecimal byte notation instead of numbers
Switches some invalid test values to be more explicit (for example a value that is supposed to be invalid hexadecimal with just one letter not being hex was not obvious, so I replaced it with notHexadecimal to be clearer and removed the comment that explained that it was not hexadecimal)
Fixes flakey test in the retriever
Makes all tests parallelized (except those that are being changed in #223)
Wrap a bunch of errors that were not wrapped / use errors where they were forgotten
... And probably some more tiny fixes I don't remember of.

Fixes #232
Notes

~If #223 gets merged before this PR, we will need to add the parallelization changes to it and to reorder its new interfaces alphabetically.~

Checklist

[x] Is on the right branch
[x] Documentation is up-to-date
[x] Tests are up-to-date

To be completely honest, I don't think there's much value in enforcing comment writing style. For stuff like function documentation and similar, sure. But for internal stuff, comments inside functions and oneliner comments, it just feels too rigid. Those may be small and inconsequential freedoms, but they're still freedoms.

@Maelkum: I disagree with this, and let me try to convince you about why!
When you put a comment like this:
// the following code of block does something

... you essentially limit yourself to a short snippet. I used to do that, and it usually becomes tedious. Once you develop a better understanding of the why of code comments, you will almost never want to put just a short snippet.
Case in point:
// unpack the message into the value
value, err := unpack(message)

Here it indeed makes sense to use just a snippet; but the comment is pointless. It is just redundant with what the code does. Once again, comments should talk more about the why and the how. Now, let's say you do comments that make more sense.
// here, we unpack the message into a value so that we can easily parse the status code
value, err := unpack(message)

This is completely made up, by the way, but you see the point. It's still fine, but you are now starting to have a dialogue with the code reader. You could easily imagine extending the one-liner into a paragraph. I think at that point, it's clear that writing everything in real sentences and real grammar rules becomes the superior approach, because it's more readable and thus conveys the ideas more clearly.
// Here, we unpack the message into a value, which allows us to easily parse the status code. This
// is required so that we can decide what to do with the message on the second layer.
value, err := unpack(message)

Once you get into the habit of writing code comments more like this, they become more useful and add more value. You will also stop writing one comment line for each line, and rather switch to commenting naturally coherent code blocks.
@Ullaakut: this is also, imo, a great argument towards allowing the kind of first person comments we keep arguing about. It's a story you tell, a dialogue you lead with the reader. It's pretty easy to absorb, imo, when you talk like that. And it's not always possible to be as concise without it.
Once again, comments that go into the Godoc should be more objective/factual, but for comments that explain the code flow from the inside, I think it's perfectly fine, maybe even desirable, to have such a narrator style.

@awfm9
I agree with what you're saying, but I also think it's a false equivalence. One-liner comments are not always useless and duplicate what the code is saying..
I agree that longer comments, paragraphs describing the functionality in a code block, or a complex loop, should be formatted and written in a certain way. The code in the mapper.Run() is a nice example of this. It has multiple long comments describing to the reader what the code below does (or intends to do).
For instance:
	// We start at an "imaginary" step that refers to an empty trie, has no
	// paths and no previous commit. We consider this step already done, so it
	// will never be indexed; it's merely used as the sentinel value for
	// stopping when we index the first block. It also makes sure that we don't
	// return a `nil` trie if we abort indexing before the first block is done.

But one-liner comments often clarify code when it's not immediately evident why something is done.
Exhibit A:
Outer:
	for {
		// We want to check in this tight loop if we want to quit, just in case
		// we get stuck on a timed out network connection.
		select {
		case <-m.stop:
			break Outer
		default:
			// keep going
		}

keep going is perfectly clear. We could write \\ We should keep going until we reach X. but I don't think it adds more value.
Exhibit B:
	// Any other error should not happen and should crash explicitly.
	if err != nil {
		return fmt.Errorf("could not retrieve next commit (height: %d): %w", height, err)
	}

I think // any other error should not happen along with the return is clear enough. The comments say the same thing, only in more words.
I think these are also examples of such short, simple comments that are absorbed in a split second along with the code below, and less of actual sentences:
	// Initialize storage library.
	codec, err := zbor.NewCodec()
	if err != nil {
		log.Error().Err(err).Msg("could not initialize storage codec")
		return failure
	}
	storage := storage.New(codec)

	// Check if index already exists.
	_, err = index.NewReader(db, storage).First()
	indexExists := err == nil
	if indexExists && !flagForce {
		log.Error().Err(err).Msg("index already exists, manually delete it or use (-f, --force) to overwrite it")
		return failure
	}

	// Initialize mapper.
	chain := chain.FromDisk(data)

I do completely agree with the superiority of well written comment paragraph describing the code flow or the approach. I just think both types of comments exist.

@Maelkum I would like to look at this in a differentiated way, because not all your examples are the same.

Exhibit A:
Outer:
	for {
		// We want to check in this tight loop if we want to quit, just in case
		// we get stuck on a timed out network connection.
		select {
		case <-m.stop:
			break Outer
		default:
			// keep going
		}

I agree with this, but it's an exception. This is not a code comment, as such. It's a comment that stands in for code, to explain that the absence of code in that specific case actually stands for something. There are similar cases that I would tend to agree with, such as // do nothing, or // this should never happen.

Exhibit B:
	// Any other error should not happen and should crash explicitly.
	if err != nil {
		return fmt.Errorf("could not retrieve next commit (height: %d): %w", height, err)
	}

I disagree with this one. If it's worth putting a comment before the condition clause here, it's worth making it a real code comment that documents what's special about that clause. Here, we are back in the territory of real code comments that tell the story about the why and the how, and we should use correct text formatting and grammar.

	// Initialize storage library.
	codec, err := zbor.NewCodec()
	if err != nil {
		log.Error().Err(err).Msg("could not initialize storage codec")
		return failure
	}
	storage := storage.New(codec)

	// Check if index already exists.
	_, err = index.NewReader(db, storage).First()
	indexExists := err == nil
	if indexExists && !flagForce {
		log.Error().Err(err).Msg("index already exists, manually delete it or use (-f, --force) to overwrite it")
		return failure
	}

	// Initialize mapper.
	chain := chain.FromDisk(data)

I think these are good examples of bad comments (mostly). And, as before, they should be about the why and how, and use proper text formatting. Here is an improved version:
// Here, we initialize the storage library. It's a toolbox for functions
// that save and retrieve data from a badger database and apply
// a homogeneous encoding to all interactions.
codec, err := zbor.NewCodec()
if err != nil {
	log.Error().Err(err).Msg("could not initialize storage codec")
	return failure
}
storage := storage.New(codec)

// This check ensures that no index database has been previously created
// in the given index database folder. If we have already created an index
// in the directory before, the database will contain a first height. We will
// need to set the force flag in order to overwrite previous data then.
_, err = index.NewReader(db, storage).First()
indexExists := err == nil
if indexExists && !flagForce {
	log.Error().Err(err).Msg("index already exists, manually delete it or use (-f, --force) to overwrite it")
	return failure
}

// Initialize the mapper with all of its dependencies. The mapper encapsulates
// the core business logic and will use the other components to read data from
// the spork state and write to the index.
chain := chain.FromDisk(data)

@awfm9
I mostly agree again. However, for the third example, related to the "bad comments".
// Here, we initialize the storage library. It's a toolbox for functions
// that save and retrieve data from a badger database and apply
// a homogeneous encoding to all interactions.
codec, err := zbor.NewCodec()
if err != nil {
	log.Error().Err(err).Msg("could not initialize storage codec")
	return failure
}
storage := storage.New(codec)

// This check ensures that no index database has been previously created
// in the given index database folder. If we have already created an index
// in the directory before, the database will contain a first height. We will
// need to set the force flag in order to overwrite previous data then.
_, err = index.NewReader(db, storage).First()
indexExists := err == nil
if indexExists && !flagForce {
	log.Error().Err(err).Msg("index already exists, manually delete it or use (-f, --force) to overwrite it")
	return failure
}

// Initialize the mapper with all of its dependencies. The mapper encapsulates
// the core business logic and will use the other components to read data from
// the spork state and write to the index.
chain := chain.FromDisk(data)

Don't you think the first and third comment fit better in the actual package (zbor/chain), perhaps as a documentation for the function that's being called, or just in the package docs? Else we'll be explaining the functionality of the storage library and mapper in each tool that's using it.
Using a library would somewhat imply that the user knows what it's doing, and the documentation for what the library is doing should be in the library itself.
If we remove the description of what the entities do from those comments, we once again have only "Here we initialize the storage library" and "Initialize the mapper with all of its dependencies"..

Using a library would somewhat imply that the user knows what it's doing, and the documentation for what the library is doing should be in the library itself.

There is a difference between the documentation of a package, and documenting code for others to understand. Comments like these exist for other software engineers, so they can understand the overall logic of the program without having to delve into every component. As a project grows, it's not viable for everyone to know every component anymore, so they become more and more valuable.

Okay, hard to disagree with that. I still fear that there's some potential for redundancy, bud overall it is a good thing.

I'm really not used to this style of comments, but I can see your point, sounds good to me.
Don't forget to approve the PR if it looks good to you :p It's a tough PR to maintain up-to-date since it modifies a lot of stuff across the codebase.

I'm really not used to this style of comments, but I can see your point, sounds good to me.
Don't forget to approve the PR if it looks good to you :p It's a tough PR to maintain up-to-date since it modifies a lot of stuff across the codebase.

I would approve if you would remove the re-ordering of the RPC requests.  I just do not think there is value in the change at all, and the current ordering reflects to an extend the architecture of the application.
It starts with the core indexes, last and first, which are crucial to know what requests could be valid. It then goes to the commit, which is core to indexing data. Next come all of the chain data entities, in logical order as they appear in the Flow protocol. Then come the payloads, which are not part of the protocol state, but further removed in the execution state.
This might not be perfect, but I think it has more value than alphabetically ordering things.

I would approve if you would remove the re-ordering of the RPC requests.  I just do not think there is value in the change at all, and the current ordering reflects to an extend the architecture of the application.

Done, I did it in the same order than it is on your refactor PR to minimize the potential conflicts as much as possible. Let me know if it's good for you.


[CAMEL-15276] camel-weather: properly initialize GeoLocationProvider

Fix this issue (CAMEL-15276) by using the geolocationAccessKey and geolocationRequestHostIP from the WeatherConfiguration instead of WeatherComponent
Move httpClient parameter to WeatherConfiguration
Introduce a new parameter geoLocationProvider to allow the use of a custom GeolocationProvider (the default implementation relies on a third party service i.e. the ipstack API)

This PR changes the parameters and options of the component. If that's not desirable, then I can try to just fix the issue without those changes.
PS: this component needs more work imo, for example WeatherQuery.getQuery changes some values in the WeatherConfiguration which is a side effect.

I added the note in this PR. Is that okey ?

Perfect, thanks.

Thanks

my pleasure


Allow for Jump to Definition

It would help a lot if the extension would show the diff for the PR in question but also allow for some of the IDE's feature like "Jump to Definition".
While being able to perform PRs in my IDE is very convenient, the ability to have a view of the PR in my IDE and navigate the code will be even better.

I agree as this would also help immensely for code review sessions. This is something that the web client lacks.

This looks like a duplicate of https://github.com/microsoft/vscode-pull-request-github/issues/486.
There's already some ability to do this, but it requires clicking the "Checkout" button on a pull request's overview. This brings up a new "Changes in Pull Request" view under the old tree, the diffs opened under that should support code navigation as normal.


Merging Turkie with TuringCallbacks

Ok time to get this done!
As we talked about, it would be nice to merge Turkie with TuringCallbacks.
The question is how? Regardless of the UI, which I think can be unified, the problem is with dependencies:
Turkie relies on AbstractPlotting.jl which is pretty heavy, similarly TuringCB relies on TensorboardLogger, and therefore tensorboard which is also heavy in a way.
Easiest solution would be to use Requires, but it has its issues too.
Maybe it would be nice to have some kind of Plots.jl approach where the user can decide what he wants with a function turkie() or tensorboard() or something like this.
Also I saw you have a dependency on Turing, if just rely on AbstractMCMC / DynamicPPL, this dependency is not necessary and the package could be used for other PPL no?

The question is how?

This depends on how we want to do things. One approach is to simply implemented a callable struct a la TensorBoardCallback https://github.com/TuringLang/TuringCallbacks.jl/blob/master/src/callbacks/tensorboard.jl#L129. Then we can at least share implementations of different statistic implementations, e.g. https://github.com/TuringLang/TuringCallbacks.jl/blob/master/src/stats.jl.
Ideally we'd have a shared main function to call, but it's a bit unclear to me how we'd do that at the moment. Therefore I think what I mention above is the best way to get started, and then we can figure out how to share more code between the methods later.

Turkie relies on AbstractPlotting.jl which is pretty heavy, similarly TuringCB relies on TensorboardLogger, and therefore tensorboard which is also heavy in a way.

TensorBoardLogger doesn't require tensorboard to be installed to use it. tensorboard is only needed if you want to visualize the outputs. This means that you can for example run code using TuringCallbacks.jl on a server which does not have tensorboard while still producing logs which you can look at later.
I agree though, AbstractPlotting.jl is quite a dependency if you for example only want to use the tensorboard logging.
How does the Plots.jl approach actually work? It also seems like Requires.jl would be a reasonable choice in this case since the main code will fairly light-weight and so the compile-times shouldn't be very long.

Also I saw you have a dependency on Turing, if just rely on AbstractMCMC / DynamicPPL, this dependency is not necessary and the package could be used for other PPL no?

We currently depend on Turing.jl for the following methods: https://github.com/TuringLang/TuringCallbacks.jl/blob/1a4bbc396b295833c6dc4eaa285f5c6d79b35a13/src/callbacks/tensorboard.jl#L110 and https://github.com/TuringLang/TuringCallbacks.jl/blob/1a4bbc396b295833c6dc4eaa285f5c6d79b35a13/src/callbacks/tensorboard.jl#L129. So the dependency is deliberate (this method is not available in DynamicPPL.jl or AbstractMCMC.jl).
But I agree that ideally we wouldn't need this method, so open to suggestions on how to improve this!

Regarding https://github.com/TuringLang/TuringCallbacks.jl/blob/1a4bbc396b295833c6dc4eaa285f5c6d79b35a13/src/callbacks/tensorboard.jl#L110
I don't know if it works but I got this working in Turkie :
https://github.com/theogf/Turkie.jl/blob/3990deb6f18198648cf34a595c975d2512ed792d/src/Turkie.jl#L93-L99
For the transitions extras maybe we could require the user to load Turing for it?

The issue with that approach is that all AbstractTransition will not have the θ field, which is why the _params_to_array is used in the current impl. We could indeed replace _params_to_array somehow, but it will still rely on tonamedtuple which requires Turing.jl (or one of the other packages under the Turing-umbrella; I forget which package the "base-version" of tonamedtuple(::AbstractTransition) is implemented in).

Hey so I was having a second look at the merging and it looks more and more that that merging Turkie to TuringCallbacks is not viable.
As we talked about AbstractPlotting.jl is too heavy of a dependency to have by default.
But the problem by using Requires.jl is that there is no version control, and this is a large issue with the ever changing API of Makie.
What I would propose is to go down the road of having an AbstractSamplingCallbacks package containing all the necessary methods from which TuringCallbacks and Turkie would both be depending on.


app(props, container) VS props.container

Which want do we want?
app({
  state,
  view,
  actions,
  container|root
})

VS
app(
  {
    state,
    view,
    actions
  },
  container
)

Revival of #410.

I preferred the first option, where all of the app's config is in one object.

I don't really have a strong preference, but a slight leaning toward the second form.  Because the container is not really part of the app's logic anyway. I can see situations (in particular in testing) where you get the app-definition from one place, and the container from someplace else. Combining them to execute the app is slightly easier and more natural in the second form, IMO.
The obvious con is that it makes writing HOA a little more complex. But only a very little. And the way I see it, HOA are uncommon and complex enough things anyway, we can expect a HOA author should know what they're doing.
Actually (just thought of this), even for writing HOA the second form might be helpful -- in case you're writing a HOA which takes care of setting up a container for you (for multiple apps, apps as stateful components, whatever...)
Still the difference is so small it's barely worth arguing about 😛

I support the second form as well.

Slight preference for option 2, but not strongly.

My only opinion on this matter is that container is not a part of the app wiring like view, actions and state are, so by that logic it makes sense that it is delivered separate from them.
container is also an optional argument with a reasonable default, which also fits a common pattern in JS to have such parameters following mandatory parameters to a function.

React's render() works this way too.
render(element, container)

This is a plus for newcomers from the land of React.

I find the first one more pleasing to look at when you are using all props, because of how it's formatted.
On the other hand, you don't necessarily always use a specific root|container.

but why should one compare it to React? It is not all that good.

They both make sense.. I rarely use root|container anyway (just let it fallback to document.body) so I don't really have enough experience with this to comment. the first looks more compose-able the second looks more like other frameworks do things (namely react).

At this point whicever one saves the most bytes/or is the more performant option. I don't mind either API as long as we pick one!

I think we should go with whatever makes it simple for two type of usage
a. Basic usage: I think for most users both options are simple.
b. App compose usage: I think the 2nd option may cause issues for people using/building HOA tools I guess.
So I'm now leaning towards option1 because it may increase cred in the functional programming world.

@rajaraodv Were you thinking of any issue in particular?

@zaceno Actually (just thought of this), even for writing HOA the second form might be helpful -- in case you're writing a HOA which takes care of setting up a container for you (for multiple apps, apps as stateful components, whatever...)

Could you elaborate please? 🙏

@rajaraodv Were you thinking of any issue in particular?
Nope. I am just thinking about others talking about some issue while writing HOA. I myself am not aware of it.

@rajaraodv it would be nice to use normal function composition and functional libraries for HOAs.
I'd like a final answer to this soon, so I know if I need to update all my HOAs or not 😜

@zaceno Actually (just thought of this), even for writing HOA the second form might be helpful -- in case you're writing a HOA which takes care of setting up a container for you (for multiple apps, apps as stateful components, whatever...)

Could you elaborate please? 🙏

Hehe not even sure it makes sense. Just a notion that struck me. Was thinking if like you were using hyperapp to write a standalone ui component (generic, not specifically for any framework). Then you'd be given the container by the user of your thing. Then it would very slightly more convenient just to toss that on the end of the app call, rather than first assign container in your props and then call app.
Again: my feelings on this are not strong. Just trying to brainstorm around scenarios where a custom container would be used at all. Normally no one would use it anyway and it wouldn't matter.

I think it all depends on what we want to optimize for. Do we think this feature will be most often used to add Hyperapp to an existing SPA? If so, then the second form will be more familiar to devs that already know React or other libraries with similar APIs.
However, this has a very different impact on library/HOA authors. With the first form you are already required to pass props to the next app function, so you would have to intentionally leave out container/root to break things. OTOH with the second approach you have to make a conscious effort to do the right thing by passing the second argument. The option 1 takes effort to do it wrong and option 2 takes effort to do it right. This could get annoying since most function composition helpers are designed for single argument functions. This mean rolling more of your own code where bugs can creep in 🐞
Here's a SO answer that goes into some of the details about why this is an issue: https://stackoverflow.com/a/28314380

This is how stuff looks in practice right now.
reactor({ ... })(
  devtools({ ... })(
    logger({
      log(prevState, action, nextState) {
        // ...
      }
    })(app)
  )
)(
  {
    state,
    view,
    actions
  },
  document.getElementById("root")
)

From a user experience POV, either (1) or (2) make no difference to me, other than the extra indentation. HOA authors will need to make sure to pass along the container to the app inside their HOA implementation, so it's arguably not too bad.

I dont know the status of the discussion, but for me, the HOA excuse is not real and serious.
Some weeks ago, I presented on slack my HOA snippet with the following features : scoped utilities, deeply modules ehancement, ehanced app function, etc...
And here is the revision to make it work with the container as second parameter
@@ -3,17 +3,17 @@
 function HOA(app) {
   /* HOA utilities */

-  return function enhancer(props, isModule) {
+  return function enhancer(props, container, isModule) {
     /* HOA on module props too */
     for (var name in props.modules) {
-      props.modules[name] = enhancer(props.modules[name], true)
+      props.modules[name] = enhancer(props.modules[name], container, true)
     }

     /* HOA internal */
     /* ... */

     /* HOA on module return an object */
-    return isModule ? props : app(props)
+    return isModule ? props : app(props, container)
   }
 }

So, this is not really a surprise for most of you, but I am in favor of the second version
app(
  {
    init,
    state,
    view,
    actions
  },
  container
)

Because I have two wish that match with the previous statement.
1. I want to use module and app props like the same things, Yes!
const Counter = {
  state: {},
  actions: {},
  view(state, actions) {}
}

app(Counter, container)

app({
  modules: { Counter }
  state: {},
  actions: {},
  view(state, actions) {}
}, container)

2. I want to use Hyperapp as a candidate to create WebComponents (because we are creating WebComponents at my company)
<body>
  <cool-counter id="counter1"><cool-counter>
  <cool-counter id="counter2"><cool-counter>
</body>

document.querySelectorAll('cool-counter').forEach(
  container => app(Counter, container)
)

This is pretty naive, but I show you two use cases in favor of the container as second parameter.

Is this still an open discussion? It seems like option two has been in long enough to close this now.

@okwolf I was hoping to revisit again later. I would like to know if everyone's opinion is still the same or not after modules gone.

Module was just a sugar for wiring imported state and actions in the same namespace in one line. The use case is unchanged.
Having to define the App props somewhere and attaching it to the dom elsewhere is still a good pattern for me.

At this point I’d rather leave this the way it is than ping pong back and forth with breaking changes.

any solution, @JorgeBucaran ?
(props, container) winner?

@dmitry-vakhnenko Yes, that's what it looks like.


doc: minor sudo related suggestion in commands

Add sudo in commands that require root user access.
For example, in https://github.com/openvinotoolkit/security_addon/blob/master/docs/ovsa_get_started_kvm.md
sudo dmesg | grep -i TPM

Updated as on OVSA release 2022.1.


[RFC] Setup CICD pipeline for Dashboard site

Expectations:

As RDS is following Test Driven Development also known as TDD we need to set up a testing framework for the Dashboard site.
Select a unit testing library that best suits the Dashboard site.
Implement a CICD pipeline.

Advantages of Using Jest

Can be used as a CLI when installed globally yarn global add jest or npm install jest --global npm install --save-dev jest-cli "Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy Unrestricted" - cannot be loaded because running scripts is disabled on this system

Jest can be used with WebPak, parcel, and typescript.

We can mock functions, modules/library

Jest has a platform of packages that can be used standalone. jest-changed-files, jest-diff, jest-docblock, jest-validate, etc These tools are used to make interactions with our application

Huge community to support Jest.

Has good support for JS frameworks.

It Also can be used by Puppeteer. That is being used in the Dashboard site.

It is easy to migrate to the Jest from other testing frameworks using codemods.

Disadvantages of Using Jest

Jest is a comparatively new framework. Although it has vast community support it is still getting mature.

Developers sometimes complain about the learning curve.

Mocking tests make the application slow.

Snapshot testing with jest is not so feasible, for huge codebases.


Update with Improved Session Handling

We just rolled out a new version of the tutorial with a few minor updates (details below). If you are looking for the older version of the tutorial, we have a hosted version here.
Updates

Serverless API

Setting email as a username in Cognito User Pool

React Client

Fixed logout not clearing session
Removed need to store the user token
Fixed session expires if app not refreshed
Removed extra withRouter imports
Formatted code snippets

Email as a username
Previously, we were manually setting using the email as a user’s username. There is one major issue with this; the username cannot be changed in your Cognito User Pool. AWS fixed this by adding the option of using the email as the username while logging in and signing up. Internally, the User Pool uses an auto-generated value as the real username. This means that we won’t need to do any extra work to allow users to login with their email. Of course, you can skip this option entirely if you are not planning on allowing users to login with their email.
Auth/Session flow changes
The previous setup had a couple bugs that a few folks had noticed. If the app is not refreshed for over an hour the session would expire and we would get an ugly error if we tried to carry out any action. This was because we were only generating the user token when we refreshed the page (or when the App component loaded). A better flow here would be to ensure that the session is valid both at load time and when we are about to make a request to AWS (our API or any other resources). This also means that we don’t need to store the user token explicitly in our App state since we are going to get it every time we make a request. We also made it so that we get the user token and the AWS temporary credentials at the same time. This is more inline with what the Cognito JS SDK docs suggest as well.
We also had an issue where logging out was not clearing the AWS temporary credentials completely. This was happening because we were deleting the credentials object in the AWS JS SDK but we need to set them to an empty value so that the browser LocalStorage can be reset as well. You can see the exact steps here.

While these changes aren’t very significant, the flow has changed. And as a result if you were working through the frontend portion of the tutorial, you might run into a few issues. To help you with this we have hosted past version in case you need them for reference. You can view all the different updates here.

Would it be possible to get some sort of list of which specific pages in the tutorial (Or files in the app) were changed with 1.1 so that we can go back and implement the changes ourselves? I would prefer to manually make the specific edits to my current project rather than importing the new version, because that way I will not be blindly running code that I don't understand. This would also be nice for users who have use this stack as a starting point for a project.

@bharloe The versions are tagged in the repo, so you can do a diff between the versions (https://github.com/AnomalyInnovations/serverless-stack-com/compare/v1.0...v1.1). The only tricky thing about this is that I made quite a few cosmetics style changes to the code samples, so a lot of files have changed.
The API code base itself has not changed, but the way we set up the User Pool has. Specifically, the first few steps where we set the email as username attribute - https://serverless-stack.com/chapters/create-a-cognito-user-pool.html. And since our API itself does not deal with the user accounts directly, the only affected portion is the frontend.

Can somebody speak as to why this issue is still in the open state? I'm totally confused, the guide is at v7.2 by now...


client simulator: Add CMake support

CMakeLists.txt was missing for the client simulator.
Signed-off-by: Michael Soares<EMAIL_ADDRESS>

@krocard and @dwagner please review

Thanks Michael :) just a "few" comments.

@soaresm ok, I got it. There is indeed a bug in older CMake's module for finding the python interpreter that makes impossible to get another version than the system's default one.
It is fixed is newer 2.8 CMake releases (I've tested 2.8.11). See https://github.com/dawagner/parameter-framework/tree/soaresm-cmake_clientSimulator
Please also remember to rename the python module folder from "clientSimulator" to "clientsimulator" (as specified by https://www.python.org/dev/peps/pep-0008/#package-and-module-names)


remove the default value for plot.

Warning : this bring to an API modification BaseMaskToolsDockWidget(self, parent=None, plot=None, name='Mask') -> BaseMaskToolsDockWidget(self, plot, parent=None, name='Mask')

This modification is including a modification of the API BaseMaskToolsDockWidget(self, parent=None, plot=None, name='Mask') -> BaseMaskToolsDockWidget(self, plot, parent=None, name='Mask').
what is the procedure in this case: adding a warning in the documentation ? Something more ?

As far as I know we had decided to have parent=None as the first argument of all the widgets for easy promotion in the designer. Why is the modification?

ok, my bad I though we wanted to have parent as the first parameter with a default value and not the first in any case.
In this case an assert with doc should do it.


Upgrade react-native-mmkv-storage dependency

Affected areas:

Persistent storage

Test from upgrade:

previous data should be kept (upgrade smoothly)

Test common:

As new data gets added closing and opening keeps new data

Building app in separate branch.

Successfully building:
https://circleci.com/gh/mattermost/mattermost-mobile/9347
https://circleci.com/gh/mattermost/mattermost-mobile/9346

Artifact links:
https://9347-70265724-gh.circle-artifacts.com/0/~/mattermost-mobile/Mattermost_Beta.ipa
https://9346-70265724-gh.circle-artifacts.com/0/~/mattermost-mobile/Mattermost_Beta.apk

Will Add Reviews Complete once Dev Review is done.


Enable make docs in make generate

What this PR does / why we need it:
Sometimes our api-reference is outdated, this PR adds make docs in make generate so our lint can against that.
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #
Special notes for your reviewer:
Documentation:

Does this PR introduce a user-facing change?:

NONE

/retest

/retest

/lgtm


Keycloak certificates no longer work

Ldap connection fails as a result

Probably related to #115

This seems to be relevant.  https://askubuntu.com/questions/1004745/java-cant-find-cacerts
The global /etc/ssl/certs/java/cacerts was showing an older cert for the DC.  Not sure why.
The fix,
sudo dpkg --purge --force-depends ca-certificates-java

sudo apt-get install ca-certificates-java

Moving out of the 0.3 release, this is going to be a bigger problem than I anticipated.  There are two problems:

Keycloak isn't using the cert store that is loaded for connecting to the ldap servers
A separate issue, the system wide java cacerts file is not properly updating when new certs are generated.


sync: cleanup and implement fsync if files are given

Changes:

The files are PathBuf now, allowing invalid UTF-8
We've got fsync now if files are given but neither --data or --file-system is passed.
We no longer check the paths up front, but while opening the paths.
Our functions are no longer marked unsafe, because they aren't :)
--data and --file-system are hidden when they are not supported
--data and --file-system trigger an error when they are used, but not supported
Some functions were (I think) available on more platforms that what we were using, so I tried to remove from cfg gates.

I can't be bothered with also fixing Windows right now :)
I tried to use nix, but it was sadly missing some Android stuff, which I've made a PR for: https://github.com/nix-rust/nix/pull/2296.

Meh, I'll fix Mac soon

@tertsdiepraam ping ? :)

inactive for a while, closing. cc @tertsdiepraam