tf destroy fails to remove aws_auth: unauthorized

I have issues
I'm submitting a...

[x ] bug report
[ ] feature request
[ ] support request - read the FAQ first!
[ ] kudos, thank you, warm fuzzy

What is the current behavior?
When I do a destroy operation, I receive
Error: Unauthorized

The only remaining piece of state is the aws_auth module:
 # module.kubernetes_cluster.module.eks.kubernetes_config_map.aws_auth[0] will be destroyed

Environment details
module "eks" {
  source = "terraform-aws-modules/eks/aws"

  cluster_name     = var.cluster_name
  cluster_version  = "1.18"
  subnets          = module.vpc.private_subnets
  iam_path         = "/eks/"
  write_kubeconfig = false

  kubeconfig_aws_authenticator_command = "aws"
  kubeconfig_aws_authenticator_command_args = [
    "--region",
    var.region,
    "eks",
    "get-token",
    "--cluster-name",
    var.cluster_name,
  ]

  cluster_encryption_config = [
    {
      provider_key_arn = aws_kms_key.eks.arn
      resources        = ["secrets"]
    }
  ]

  tags = merge(
    var.tags
  )

  vpc_id = module.vpc.vpc_id

  worker_groups = [
    {
      name                 = "worker"
      instance_type        = var.worker_instance_type
      asg_desired_capacity = var.number_of_workers
      asg_max_size         = 5
      iam_role_id          = "k8s-node"
    }
  ]

  workers_additional_policies = [
    "arn:aws:iam::aws:policy/AmazonSSMManagedInstanceCore",
    aws_iam_policy.nodes-ec2-policy.arn
  ]
}

Affected module version: 3.2.1
OS: osx 10.15
Terraform version: 0.14.3

I'm also getting this error... using a bare-bones install:
data "aws_eks_cluster" "cluster" {
  name = module.my-cluster.cluster_id
}

data "aws_eks_cluster_auth" "cluster" {
  name = module.my-cluster.cluster_id
}

provider "kubernetes" {
  host                   = data.aws_eks_cluster.cluster.endpoint
  cluster_ca_certificate = base64decode(data.aws_eks_cluster.cluster.certificate_authority.0.data)
  token                  = data.aws_eks_cluster_auth.cluster.token
  load_config_file       = false
  version                = "~> 1.9"
}

module "my-cluster" {
  source          = "terraform-aws-modules/eks/aws"
  cluster_name    = var.cluster_name
  cluster_version = "1.18"
  subnets         = var.subnet_ids
  vpc_id          = data.aws_vpc.default.id
  worker_additional_security_group_ids = [ var.worker_security_group_id ]

  worker_groups = [
    {
      instance_type = var.worker_instance_type
      asg_max_size  = 5
    }
  ]
}

my terraform destroy stops with an Unauthorized error
...module.my-cluster.aws_security_group_rule.workers_egress_internet[0]: Destruction complete after 2s
module.my-cluster.aws_security_group_rule.workers_ingress_cluster[0]: Destruction complete after 3s
module.my-cluster.aws_security_group_rule.workers_ingress_cluster_https[0]: Destruction complete after 4s

Error: Unauthorized

Releasing state lock. This may take a few moments...
[terragrunt] 2020/12/28 13:45:54 Hit multiple errors:
exit status 1

with TF_LOG=TRACE, I can see that:
2020/12/28 14:15:22 [TRACE] dag/walk: visiting "provider[\"registry.terraform.io/hashicorp/aws\"] (close)"
2020/12/28 14:15:22 [TRACE] dag/walk: upstream of "meta.count-boundary (EachMode fixup)" errored, so skipping
2020/12/28 14:15:22 [TRACE] vertex "provider[\"registry.terraform.io/hashicorp/aws\"] (close)": starting visit (*terraform.graphNodeCloseProvider)
2020/12/28 14:15:22 [TRACE] GRPCProvider: Close
2020-12-28T14:15:22.924-0800 [WARN]  plugin.stdio: received EOF, stopping recv loop: err="rpc error: code = Unavailable desc = transport is closing"
2020-12-28T14:15:22.928-0800 [DEBUG] plugin: plugin process exited: path=.terraform/providers/registry.terraform.io/hashicorp/aws/3.22.0/darwin_amd64/terraform-provider-aws_v3.22.0_x5 pid=5933
2020-12-28T14:15:22.928-0800 [DEBUG] plugin: plugin exited
2020/12/28 14:15:22 [TRACE] vertex "provider[\"registry.terraform.io/hashicorp/aws\"] (close)": visit complete
2020/12/28 14:15:22 [TRACE] dag/walk: upstream of "root" errored, so skipping

Error: Delete "http://localhost/api/v1/namespaces/kube-system/configmaps/aws-auth": dial tcp [::1]:80: connect: connection refused

Same error here, we need to change depends_on in kubernetes_config_map.aws_auth in aws-auth.tf file. It works fine when creating cluster (terraform apply) as there is this null_resource.wait_for_cluster[0] which curls cluster, but when destroying aws_auth is destroyed after the cluster which is impossible as the cluster is no longer there. The same applies if you have kubernetes/helm provider and you want to install e.g chart, then without doing
resource "helm_release" "etwas" {   depends_on = [module.eks.kubernetes_config_map.aws_auth[0]] } destroy command will leave helm_release.etwas and module.eks.kubernetes_config_map.aws_auth[0] hanging (error).

I experienced this same issue. After retrying the terraform destroy command it often deletes the EKS cluster while the kubernetes and helm resources are left behind in the state. This also leaves behind the AWS volumes and load balancers managed by Kubernetes. It really seems like the cluster is being destroyed before the resources. Versions before Terraform v14 seemed to implicitly add a dependency so that the Kubernetes resources are destroyed before the EKS cluster. After upgrading Terraform and the modules this issue arose. I also tried to build the Terraform development branch (commit 44aeaa59e70f416d582ed3ceccad7f7945f03688) from source and use this modules Github master branch, but the issue is still present.

@SirBarksALot Good that you noticed that module.eks.kubernetes_config_map.aws_auth[0]: Destroying... [id=kube-system/aws-auth] is ran before the Kubernetes resources are destroyed. It explains why the authentication details are no longer available afterward.
Your workaround with depends_on doesn't seem to work for me.

I am running into the same and related issues destroying this module with the terraform:light docker image.
I believe this is related to #978 , but I have not found any workarounds that work for automation purposes.

I am running into the same and related issues destroying this module with the terraform:light docker image.
I believe this is related to #978 , but I have not found any workarounds that work for automation purposes.

@TjeuKayim I am running into the same thing.  I believe the comment from @SirBarksALot was really for the change inside the model.  For your kubernetes_ingress resource try adding the depends_on module.eks.aws_eks_cluster.this.  Hopefully, then your ingress is destroyed but it will not fix the aws_auth config map dependency issue @SirBarksALot was referring to.

@TjeuKayim I am running into the same thing.  I believe the comment from @SirBarksALot was really for the change inside the model.  For your kubernetes_ingress resource try adding the depends_on module.eks.aws_eks_cluster.this.  Hopefully, then your ingress is destroyed but it will not fix the aws_auth config map dependency issue @SirBarksALot was referring to.

I have tried many things over the last week and I came to a conclusion that it is best to create the auth config map without a help of this module (yourself) while setting manage_aws_auth=false. In my case I have helm and kubernetes resources that have depends_on=[helm_release.etwas] (so the whole module) and even then they are being destroyed after the eks destruction. I assume it is a problem with either providers and/or terraform itself. I have one more idea to try out in order to fix this issue - as I use irsa enable_irsa=true it might be a problem that the irsa resources are connected to eks in a wrong manner (I know they are as irsa resources require oicd module.eks.cluster_oidc_issuer_url). Will keep you guys updated if I find a reasonable workaround. Btw. in my case it is totally random if terraform destroy works like a charm or tumble on eks connection (i. e. auth configmap).

I have tried many things over the last week and I came to a conclusion that it is best to create the auth config map without a help of this module (yourself) while setting manage_aws_auth=false. In my case I have helm and kubernetes resources that have depends_on=[helm_release.etwas] (so the whole module) and even then they are being destroyed after the eks destruction. I assume it is a problem with either providers and/or terraform itself. I have one more idea to try out in order to fix this issue - as I use irsa enable_irsa=true it might be a problem that the irsa resources are connected to eks in a wrong manner (I know they are as irsa resources require oicd module.eks.cluster_oidc_issuer_url). Will keep you guys updated if I find a reasonable workaround. Btw. in my case it is totally random if terraform destroy works like a charm or tumble on eks connection (i. e. auth configmap).

@TjeuKayim do not worry about the loadbalancer (and probably security group) that (I assume) ingress-nginx installation creates. If we solve the dependency/auth config map problem and the helm/k8s resources will be deleted before eks, the lb and sg will be destroyed too. Just keep in mind the destruction of lb and sg takes a few seconds during which we should not destroy eks. For that I have created null_resource that awaits lb (just have to copy it and revert for destruction xD).

@TjeuKayim do not worry about the loadbalancer (and probably security group) that (I assume) ingress-nginx installation creates. If we solve the dependency/auth config map problem and the helm/k8s resources will be deleted before eks, the lb and sg will be destroyed too. Just keep in mind the destruction of lb and sg takes a few seconds during which we should not destroy eks. For that I have created null_resource that awaits lb (just have to copy it and revert for destruction xD).

Just wanted to add that our team is also experiencing this issue as well with the basic terraform example.. seems to be around 40% failure rate on destroy we get the "Unauthorized" error.. the rest of the time it works perfectly. Unfortunately this makes a cicd process very difficult so we are very eager to hear about any solutions.
For those of you who are using
terragrunt state rm module.my-cluster.kubernetes_config_map.aws_auth[0]
as a workaround are you doing this for cicd and if so do you need to do multiple destory? For example
terraform destroy (fails on unauthorized
terraform destroy (fails on unauthorized
terraform destroy (fails on localhost refused)
terragrunt state rm module.my-cluster.kubernetes_config_map.aws_auth[0]

My concern is of course do you find it's cleaning up all the terraform created resources I often had to go back into AWS manually unless I ran 'destroy' multiple times.
Thanks!

Just wanted to add that our team is also experiencing this issue as well with the basic terraform example.. seems to be around 40% failure rate on destroy we get the "Unauthorized" error.. the rest of the time it works perfectly. Unfortunately this makes a cicd process very difficult so we are very eager to hear about any solutions.
For those of you who are using
terragrunt state rm module.my-cluster.kubernetes_config_map.aws_auth[0]
as a workaround are you doing this for cicd and if so do you need to do multiple destory? For example
terraform destroy (fails on unauthorized
terraform destroy (fails on unauthorized
terraform destroy (fails on localhost refused)
terragrunt state rm module.my-cluster.kubernetes_config_map.aws_auth[0]

My concern is of course do you find it's cleaning up all the terraform created resources I often had to go back into AWS manually unless I ran 'destroy' multiple times.
Thanks!

@JohnPolansky same here, sometimes it works and sometimes it doesn't. I have a feeling that if you create a cluster and immediately destroy it then it works, however if you wait a bit the auth config is not working, I have read somewhere that this config map might have a timer? Do you exprience the same thing John?

@JohnPolansky same here, sometimes it works and sometimes it doesn't. I have a feeling that if you create a cluster and immediately destroy it then it works, however if you wait a bit the auth config is not working, I have read somewhere that this config map might have a timer? Do you exprience the same thing John?

We can try to trace down what commit to the Terraform repository exactly caused the regression. I know for sure that v0.14.3 is affected and that v0.13.5 is not affected by this issue. I didn't test the versions in between. Has anyone here experienced the issue with other Terraform versions?

We can try to trace down what commit to the Terraform repository exactly caused the regression. I know for sure that v0.14.3 is affected and that v0.13.5 is not affected by this issue. I didn't test the versions in between. Has anyone here experienced the issue with other Terraform versions?

@SirBarksALot I've seen the issue on both an example where I created the cluster then within ~1 min destroy'd it .. and I've seen the issue where I created the cluster then ~2 hours later destroyed it.. and I've seen it succeed in both cases. It's very weird. I did also read somewhere there the terraform auth is only good for 15 mins.. but I don't think that applies here as my destroy's fail after ~5-7mins.
@TjeuKayim My co-worker is on 0.14.4 and I'm on 0.14.3 and we've been experienced the "unauthorized/configmap/aws-auth" issue. We are both very eager to resolve this so if you are looking for testers when the time comes, count us in.

@SirBarksALot I've seen the issue on both an example where I created the cluster then within ~1 min destroy'd it .. and I've seen the issue where I created the cluster then ~2 hours later destroyed it.. and I've seen it succeed in both cases. It's very weird. I did also read somewhere there the terraform auth is only good for 15 mins.. but I don't think that applies here as my destroy's fail after ~5-7mins.
@TjeuKayim My co-worker is on 0.14.4 and I'm on 0.14.3 and we've been experienced the "unauthorized/configmap/aws-auth" issue. We are both very eager to resolve this so if you are looking for testers when the time comes, count us in.

+1 on this issue.  myself and one other person both hit this issue using latest version of TF

+1 on this issue.  myself and one other person both hit this issue using latest version of TF

@TjeuKayim preliminary testing shows that 0.13.6, 0.14.0 and 0.14.1 are NOT affected. Therefore an issue would appear in v0.14.2.
Testing sample size are between 2 and 5.

@TjeuKayim preliminary testing shows that 0.13.6, 0.14.0 and 0.14.1 are NOT affected. Therefore an issue would appear in v0.14.2.
Testing sample size are between 2 and 5.

@panaut0lordv @TjeuKayim on 6th try with v0.14.0 I've got this error, v0.13.6 seems fine so far (sample size 10).
@SirBarksALot @JohnPolansky - can confirm that it has not much to do with longer period elapsing, all destroys were immediate

@panaut0lordv @TjeuKayim on 6th try with v0.14.0 I've got this error, v0.13.6 seems fine so far (sample size 10).
@SirBarksALot @JohnPolansky - can confirm that it has not much to do with longer period elapsing, all destroys were immediate

@MateuszMalkiewicz  - I can confirm that my destroy are failing with not authorized in ~3-7 mins of starting them.. so no I wouldn't say it's a "longer period". As far as the versions.. it's very hard to be sure.. because "sometimes" it will succeed, i've had as many as 5 in a row destroy perfectly. I've always done the create/destroy actions right after each other, and also done them with 1hr our apart and had the destroy fail. Hope this helps.

@MateuszMalkiewicz  - I can confirm that my destroy are failing with not authorized in ~3-7 mins of starting them.. so no I wouldn't say it's a "longer period". As far as the versions.. it's very hard to be sure.. because "sometimes" it will succeed, i've had as many as 5 in a row destroy perfectly. I've always done the create/destroy actions right after each other, and also done them with 1hr our apart and had the destroy fail. Hope this helps.

it seems like there's a race condition where the cluster is destroyed before all of it's dependencies.  in our case it was things like kubernetes_namespaces, cluster role bindings, config_map, and the cluster_role that were getting "stranded", because the cluster itself was already gone.
maybe a conditional of some sort could be added to ensure all the cluster parts are destroyed before destroying the cluster itself?  i have no idea how complicated that would be, sorry if i'm over-simplifying this.

it seems like there's a race condition where the cluster is destroyed before all of it's dependencies.  in our case it was things like kubernetes_namespaces, cluster role bindings, config_map, and the cluster_role that were getting "stranded", because the cluster itself was already gone.
maybe a conditional of some sort could be added to ensure all the cluster parts are destroyed before destroying the cluster itself?  i have no idea how complicated that would be, sorry if i'm over-simplifying this.

We've managed to figure this out. In terraform 0.14+ destroy command no loner refreshes the state of resources before generating execution plan (like it did in 0.13.X). terraform apply still does the refresh, that's why this issue is more frequent the more time elapses after applying.

We've managed to figure this out. In terraform 0.14+ destroy command no loner refreshes the state of resources before generating execution plan (like it did in 0.13.X). terraform apply still does the refresh, that's why this issue is more frequent the more time elapses after applying.

We've managed to figure this out. In terraform 0.14+ destroy command no loner refreshes the state of resources before generating execution plan (like it did in 0.13.X). terraform apply still does the refresh, that's why this issue is more frequent the more time elapses after applying.

This seems plausible, but are you able to reproduce the success of refresh at making destroy work, and verify that the refresh is in fact the action which resolves this? I'd like to validate the theory, but with apply of EKS taking ~15min each time, it's a tedious process to run through different scenarios (apply/destroy, apply/refresh/destroy, doing so on 0.13.x and 0.14.y, etc.).

We've managed to figure this out. In terraform 0.14+ destroy command no loner refreshes the state of resources before generating execution plan (like it did in 0.13.X). terraform apply still does the refresh, that's why this issue is more frequent the more time elapses after applying.

This seems plausible, but are you able to reproduce the success of refresh at making destroy work, and verify that the refresh is in fact the action which resolves this? I'd like to validate the theory, but with apply of EKS taking ~15min each time, it's a tedious process to run through different scenarios (apply/destroy, apply/refresh/destroy, doing so on 0.13.x and 0.14.y, etc.).  On refresh we get a new token and everything's fine and dandy… Well, if you're good with refreshing first. I have yet to test on some pipeline-like scenario (init and destroy).
Actually the way we found out about this wasn't exactly EKS setup but destroying some of the EKS workshop usage examples using above mentioned EKS, i.e. separate module that references EKS module (so mixed AWS, k8s and helm resources but without the risk of getting rid of aws-auth/control plane).

@clebio for now the answer is yes. On refresh we get a new token and everything's fine and dandy… Well, if you're good with refreshing first. I have yet to test on some pipeline-like scenario (init and destroy).
Actually the way we found out about this wasn't exactly EKS setup but destroying some of the EKS workshop usage examples using above mentioned EKS, i.e. separate module that references EKS module (so mixed AWS, k8s and helm resources but without the risk of getting rid of aws-auth/control plane).

So I thought I would chime in to say thanks for @MateuszMalkiewicz suggestion for the terraform destroy I just did some testing.. and I can say that the refresh does appear to resolve the destroy failing. First I downloaded Terraform 14.5 and confirmed that I was still getting the Unauthorized issue on the destroy and sure enough 3 create/destroy and 3 unathorized failures. Next mytesting was based on standing up 6 EKS clusters with terraform each in a different AWS region. I ran the multiple times for a total of 18 build/refresh/destroy. All 18 worked without failure. I did one final set of 6 where I did the create/resfresh/waited 30 mins/destroy and guess what unauthorized so the refresh does appear to work I've never had 18 pass 100% before, the only catch is make sure the refresh is immediately before your destroy don't delay.
Obviously this feels more like a workaround than a fix, it seems like Terraform should be handling this for us but it is useful, Thanks to everyone participating in this.

So I thought I would chime in to say thanks for @MateuszMalkiewicz suggestion for the terraform destroy I just did some testing.. and I can say that the refresh does appear to resolve the destroy failing. First I downloaded Terraform 14.5 and confirmed that I was still getting the Unauthorized issue on the destroy and sure enough 3 create/destroy and 3 unathorized failures. Next mytesting was based on standing up 6 EKS clusters with terraform each in a different AWS region. I ran the multiple times for a total of 18 build/refresh/destroy. All 18 worked without failure. I did one final set of 6 where I did the create/resfresh/waited 30 mins/destroy and guess what unauthorized so the refresh does appear to work I've never had 18 pass 100% before, the only catch is make sure the refresh is immediately before your destroy don't delay.
Obviously this feels more like a workaround than a fix, it seems like Terraform should be handling this for us but it is useful, Thanks to everyone participating in this.

Doing terraform refresh before destroy also solves the problem for my CI/CD pipeline.
Thanks Mateusz Małkiewicz for sharing this workaround!

Doing terraform refresh before destroy also solves the problem for my CI/CD pipeline.
Thanks Mateusz Małkiewicz for sharing this workaround!

Yes, seeing this same issue while running the destroy in CI/CD with Terrafrom 14.5, the workaround which is suggested above is kind of solving the issue.
Running terraform refresh just before terraform destroy is working.
However till v13.6 this issue is not reproducible.

Yes, seeing this same issue while running the destroy in CI/CD with Terrafrom 14.5, the workaround which is suggested above is kind of solving the issue.
Running terraform refresh just before terraform destroy is working.
However till v13.6 this issue is not reproducible.

I think we should put some note in the documentation that this part is extremely error prone. I am very thankful for the module you've guys created and continue supporting as we have already made 40-50 EKS installations with it, but this part is continuously the biggest issue (and looks like it simply some terraform architecture problem).
Managing it outside of terraform should probably be the most stable solution.

BTW, refresh fix doesn't work after you've already encountered Unathorized issue:

BTW, refresh fix doesn't work after you've already encountered Unathorized issue:

Hey Bohdan, thanks for piping in. Just to be sure, have you checked if you're using same IAM user/role that was used for cluster creation and you're calling control plane from an endpoint that's allowed?
Sometimes VPN can cause you to use something else, also I found it's best to rely on data source for kubernetes provider auth info (not kubeconfig).

This seems to be a terraform (and not a module / provider) issue. I experience same problem ok Google / GKE.
I have opened an issue for terraform itself: https://github.com/hashicorp/terraform/issues/27741 please post more infos there (and vote for it! ;) )

I have this issue with Terraform v0.14.7

@bohdanyurov-gl

BTW, refresh fix doesn't work after you've already encountered Unathorized issue:

This is surely because the cluster has already been deleted by the time the unauthorized hits (this is the cause of the unauthorized!). Refresh is useless at that point.

I've had a 100% failure rate with terraform 1.14.7.  I found that running
terraform state rm module.k8s.module.eks.kubernetes_config_map.aws_auth[0]

ahead of the destroy works 100% of the time. The config map just exists on the cluster so when it's destroyed so is the config map. I would love to not have to remember to run this command.

Terraform v0.14.6 is also affected by this issue

Facing same issue in terraform version 0.14.4

I've seen this a lot in my work with the Kubernetes provider. The problem is that the data source containing the EKS credentials isn't being refreshed prior to destroy, and so the Kubernetes provider uses default values (like localhost) to attempt to connect to the cluster. The fix for this has merged upstream. It's available in starting in terraform 0.15-beta1.

@dak1n1 huh, hope we finally get this fixed.
Additionally to aws-auth, I get same thing for other resources inside kubernetes cluster (using depend_on eks module)
Like 6 helm_releases, namespaces ..
Is this goes back to the same issue ?

Im having a problem with unauthorization with the aws auth when i apply.
https://github.com/terraform-aws-modules/terraform-aws-eks/issues/1287

@kaykhancheckpoint, as mensioned earlier by dak1n1 (see https://github.com/terraform-aws-modules/terraform-aws-eks/issues/1162#issuecomment-800365918),  you might want to try v0.15-beta1 of terraform binary 👍
I did not tried it yet but I hope it helps 😄

Indeed, v0.15 does fix the issue of terraform destroy not refreshing the token (thanks a lot @dak1n1 for the info).
At the same time, we can still run into the issue when resource deletion/creation takes longer than 15 minutes.
For example, we have a CI/CD pipeline which only runs terraform apply/delete after someone clicked on a trigger button (after he /she reviewed the planned changes from terraform plan). In case the review takes long the token expires.
Please note that the kubernetes provider documentation mentions to use exec plugin to fix such issues:

Some cloud providers have short-lived authentication tokens that can expire relatively quickly. To ensure the Kubernetes provider is receiving valid credentials, an exec-based plugin can be used to fetch a new token before initializing the provider.

But this requires to have a full blown aws cli available where you run terraform. As we do not want this in our CI pipeline (129MB + groff and less packages as dependencies and it also requires proper glibc so, apline image does not work), I create a very simple and small go app that can be used in the exec of the kubernetes provider. It can be built into a static binary and takes only 13MB space. You can find it here: terraform-kubernetes-provider-eks-exec-plugin.
It works very well for us and I hope it helps to others as well.

You can also use the aws-iam-authenticator binary. It's probably a bit safer to have in CI than the full aws CLI.  https://docs.aws.amazon.com/eks/latest/userguide/install-aws-iam-authenticator.html
Looks like the binary is about 39MB in size. https://github.com/kubernetes-sigs/aws-iam-authenticator/releases/tag/v0.5.2

We use kubergrunt eks token in the exec-plugin command. The binary is also around 40MB in size, and has other useful EKS functionality.

@dak1n1, good to know that. I did not look into what can the binary do but actually, I am using their go token package to generate and fetch the token. :)

@daroga0002 I already use the depend_on block, but thanks.


WIP: Grading Controller

Fixes #67

Pull Request Test Coverage Report for Build 586

0 of 12 (0.0%)  changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-3.3%) to 93.714%

Changes Missing Coverage
Covered Lines
Changed/Added Lines
%

lib/cadet/assessments/assessments.ex
0
2
0.0%

lib/cadet_web/controllers/grading_controller.ex
0
5
0.0%

lib/cadet_web/views/grading_view.ex
0
5
0.0%

Totals

Change from base Build 583:
-3.3%

Covered Lines:
328

Relevant Lines:
350

💛  - Coveralls


Remove remaining AsyncUsageAnalyzers

Are the AsyncUsageAnalyzers rules in BotBuilder-DotNet.ruleset still used?
Also, it looks like AdaptiveExpressions.Tests is still referencing:
<PackageReference Include="AsyncUsageAnalyzers" Version="1.0.0-alpha003">
Can this be changed as well?

Hi Eric,
Thanks for pointing this out to us, we reviewed these references and indeed, they were obsolete.
We removed them in our latest commit.
Thank you!

Changes pushed to MS PR# 5068.


Skip Vue.js compilation within  blocks

Adding this attribute would go against CommonMark spec – but you wanted to add this for your own use then this is probably best implemented via an extension :)


Let's start closing some of the issues for 1.1.6

The travis error looks like some mysql repo change or issue, but not a problem with the code.

Yeah... oh well, I'll merge it. :innocent:


Improvements in handling TenantNotFound error

Description
Changes proposed in this pull request:

Tests for RuntimeExists method added
Changed log level and messages in Executor

Related issue(s)

/retest

/retest


pass params as a dict into policy and state update functions

In my code, system parameters are passed (as params) into policy and state update functions as a list with a single dictionary item. For example, params might look like:
[
  {
    'a': 1,
    'b': 2,
    ...
  }
]

This feels unintuitive, and necessitates something like params, = params (shorthand for params = params[0]) at the top of each function.
Separately, this issue was discussed here.
This PR may fixes this issue for me, in EXEC_MODE.single_proc mode, version 0.4.28.
Before considering a merge, a discussion should be had about whether this is expected to work across the gamut of execution modes.

This is solved by PR #315


[D&D 5e OGL by Roll20] Ranger class issues

Hello,
the Ranger class has issues when creating a character via Charactermancer.

Favored Enemy (Humanoid) fields display as blank after you type data in & move onto the next step. It saves properly though.
At level 2, there's some "undefined spell list" under the level 1 spell list.
at global attack modifier there's "undefined" or "[]" modifier without any description
when choosing (at least) the Forest favored grounds, it says "undefined"

Greetings,
I added these to our backlog and we'll take a look at them soon.

Closing this ticket as I cannot reproduce these issues, and I believe that they have been resolved. If there are unresolved problems, please consider opening a new issue.


Flaky test_browsingContext_create_withUserGesture_eventsEmitted

I have seen the test fail multiple times in the past few days tests/browsing_context/test_create.py::test_browsingContext_create_withUserGesture_eventsEmitted

https://github.com/GoogleChromeLabs/chromium-bidi/pull/1874


fix(Modal): prevent resize loop for full screen height

This fixes the bug reported here.
My assumptions:

It is a good thing to allow consumers of Modal to override the full screen modal to fill the height of the screen.
The change does not materially affect the existing functionality. The scrolling class will continue to be set on modals which are larger than window.innerHeight.

Codecov Report

Merging #3024 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #3024   +/-   ##
=======================================
  Coverage   99.96%   99.96%
=======================================
  Files         163      163
  Lines        2743     2743
=======================================
  Hits         2742     2742
  Misses          1        1

Impacted Files
Coverage Δ

src/modules/Modal/Modal.js
100% <100%> (ø)
:arrow_up:

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 dfbb52a...8b83a83. Read the comment docs.


Onlyoffice in Nextcloud opens new Tab

When using Onlyoffice in Nextcloud opening a document always opens a new tab in the browser. I am pretty sure that this is intended. It would be nice to have a configuration option to change that...

Hello @Twilek-de
Thank you for the remark, we will take the issue into consideration


ThemeResource doesn't work if used for an element inside SwipeItems

Describe the bug
ThemeResource is not evaluated again when theme is changed when the element is inside SwipeItems.
Steps to reproduce the bug

Create a blank C# app
Paste the following in MainPage.xaml

<Page <!-- Omitted -->>
    <Page.Resources>
        <DependencyObjectCollection x:Key="collection">
            <SwipeItem x:Name="textBlock2" Foreground="{ThemeResource TextFillColorPrimaryBrush}"/>
        </DependencyObjectCollection>
        <!-- Uncomment this and comment the above to reproduce the issue -->
        <!--<muxc:SwipeItems x:Key="collection">
            <muxc:SwipeItem x:Name="textBlock2" Foreground="{ThemeResource TextFillColorPrimaryBrush}"/>
        </muxc:SwipeItems>-->
    </Page.Resources>
    <StackPanel>
        <Button Content="Reset" Click="OnReset"/>
        <Button Content="Apply Color" Click="OnApplyColor"/>
        <TextBlock x:Name="textBlock1" Foreground="Red" Text="Hello world"/>
    </StackPanel>
</Page>

Add the following to MainPage.xaml.cs

private void OnApplyColor(object sender, RoutedEventArgs e)
{
    textBlock1.Foreground = textBlock2.Foreground;
}
private void OnReset(object sender, RoutedEventArgs e)
{
    textBlock1.Foreground = new SolidColorBrush(Colors.Red);
}

Run the app. Click apply color. Observe text color is changed to current theme's foreground
Change theme. Click reset then apply color again. Observe text color is changed to current theme's foreground
Close the app and comment/uncomment portion of xaml as described. Essentially, Switch the collection type from DependencyObjectCollection to SwipeItems.
Repeat (4)~(5). Observe after changing theme text color isn't changed to current but last theme's foreground. IOW ThemeResource isn't working

Expected behavior
ThemeResource work.
Screenshots
No response
NuGet package version
Microsoft.UI.Xaml 2.7.1
Windows app type

[X] UWP
[ ] Win32

Device form factor
Desktop
Windows version
Windows Insider Build (22610.1)
Additional context

SDK is 22000
As noted in the comment, only

DependencyObjectCollection work correctly with list element with ThemeResource ✅
SwipeItems didn't work ❌
I tried changing SwipeItems to implement IObservableVector (need to modify custom build task) and it didn't work either ❌
Tried to implement a custom collection, didn't work either ❌

This bug affects SwipeControl, obviously. I encountered this issue when trying to make a swipe item always use the same text color even with RevealMode::Execute.
Minimal repro: https://github.com/roxk/ThemeResourceForDoInsideCollectionBug

Workaround
Do not declare SwipeItem inline in SwipeItems. Declare SwipeItem separately and add the item to SwipeItems in code behind.

It looks like SwipeItems is inheriting from DependencyObject here:
https://github.com/microsoft/microsoft-ui-xaml/blob/77111484445727c7e29a9997344485cdad031fbf/dev/SwipeControl/SwipeControl.idl#L29
However, it should probably be instead inheriting from DependencyObjectCollection, looks like the wrong base class was used. This is going to interfere with how the xaml engine can walk the Visual Tree to hook up resources and bindings and such properly. We've had to deal with this in the Toolkit for our AnimationSet class and such in the past.
Related to #931 as well, as since DependencyObjectCollection isn't generic, it does make type safety harder.

Note that no xaml collection type (e.g. BlockCollection) inherits from DependencyObjectCollection. WUX's SwipeItems also didn't work and is inheriting from DependenbyObject as well. In fact, if you look into the doc of DOC, there is 0 inherited classes.
If this is intended behavior that all collection type must inherit DOC, all of WUX and MUX's collection types have this bug. And the doc needs to be updated to reflect this requirement as this requirement is no where to be seen before.


doc: Add example for Image widget to API docs

I don't think this new example is very appropriate.
Specifically, it references a Container first and has conditional compilation flags for Wasm support (which should eventually be an implementation detail).
Maybe we could think of a better one? What prompted you to add this example?

I don't think this new example is very appropriate.
Specifically, it references a Container first and has conditional compilation flags for Wasm support (which should eventually be an implementation detail).
Maybe we could think of a better one? What prompted you to add this example?

I might be able to think of a better example as I learn more about iced. I get what you're saying about it not being a good example for the API docs. Maybe best just to close this PR and if I get any ideas as I move forward, I'll ask about them in the iced Zulip server first. Sound good?

@andy5995 Sounds good! Thank you.


Use CMAKE_POSITION_INDEPENDENT_CODE to force fPIC option (previously …

…forced for x86_64, now also needed for ARM).
Change forces update to CMake 2.8.9 as minimum required version

Documented in #6


Should None values in data be handled differently?

Currently, if data contains None values they are passed on to urllib.urlencode as normal. The behaviour of urllib.urlencode is to always call str() on the key/value.
This results in the following:
urllib.urlencode({'key': None})
# 'key=None'

To me this seems like odd behaviour. If anything it would make sense for it to result in 'key='.
One option would be to do something like v = v or '' before encoding the data.
Alternatively exceptions could be raised if a key or value is None.
Does this sound like something that should be changed?

@johtso Strange thing. I got notification about your post which begins with *Ah yes, of course, forgot about 0. What about False? if (v is None) or (v is False): v = * but before reading it I edited my previous post here and now I don't see your post from notification...

Ah, I deleted it while reconsidering the problem :) Sorry for the confusion..
Translating None to '' seems to make sense, but False doesn't really have any obvious translation..

@kennethreitz, what do you think?

This should not be happening ;)

Maybe keys with None value should not be present in the request at all?
As to True/False I think mapping them to 1/0 sounds reasonable.
This could (should?) be implemented as a hook.

Looks like I've misunderstood things a bit.
'headers', 'cookies', 'auth', 'timeout', 'proxies', 'hooks', 'params', 'config' all have keys with None values removed from them.
data is the only kwarg that doesn't have keys with None values removed from it, due to no merging going on. Hence seeing urllib.urlencode translating them to key=None.
So the only question is, do we want to remove keys with values of None from data as we do elsewhere, leave the current behaviour (str(None)), or replace None with ''.

The only reason removal upon None is because of kwarg merging when sessions are involved. This only matters for params.
I'd like to not make any changes to the behavior of urlencode of possible. No surprises that way :)

Yeah, I think that's probably wise. If you want keys without values in your data, convert Nones to '' before passing them to your request.

I agree with @piotr-dobrogost, keys with None values should be removed like the other maps.

The only purpose of removal upon None is because of kwarg merging when sessions are involved.

Well, regardless of the reason it's convenient and logical.

This only matters for params.

As this issue shows this matters not only for params. Current behavior of having  key=None as a payload of request does not make sense.

I vote that this should behave how every other browser behaves, which is to just send across an empty string. (Just spent an hour debugging this issue with James Cleary).
Cal

FYI - to anyone else who is having this issue, we hotfix'd it using the following:

    @staticmethod
    def _encode_params(data):
        """Encode parameters in a piece of data.

        If the data supplied is a dictionary, encodes each parameter in it, and
        returns a list of tuples containing the encoded parameters, and a urlencoded
        version of that.

        Otherwise, assumes the data is already encoded appropriately, and
        returns it twice.
        """

        if hasattr(data, '__iter__') and not isinstance(data, str):
            data = dict(data)

        if hasattr(data, 'items'):
            result = []
            for k, vs in list(data.items()):
                for v in isinstance(vs, list) and vs or [vs]:
                    if v == None:
                        result.append((k.encode('utf-8') if isinstance(k, str) else k,
                                   ""))
                    else:
                        result.append((k.encode('utf-8') if isinstance(k, str) else k,
                                   v.encode('utf-8') if isinstance(v, str) else v))

            print result
            return result, urlencode(result, doseq=True)
        else:
            return data, data

@foxx If you want to send empty string as a value of some param you should set its value to empty string. Why should requests send key='' when you pass key=None and not skip this param altogether?
btw, using == when comparing with None is bad style.

Why should requests send key='' when you pass key=None and not skip this param altogether?

Guess my reply is, why SHOULDN'T it? It's a 50/50 split, and really comes down to the decision of the core maintainer I guess.

using == when comparing with None is bad style.

You're right, I probably should have used "is None" - I'll patch our side - thanks.

It's not 50/50 split if you take into consideration some other aspects. In case of params requests already deletes keys with value set to None so for sake of being uniform here it should delete them too.


Binary Serializer Option issue.

I have a binary format that I'm writing a serializer for. There's an odd method of handling Option<T> in containers like structs, tuples, and seqs.
Suppose we had the following struct:
struct Foo {
    a: Option<i32>,
    b: Option<f32>,
}

This is serialized as follows.
Foo { a: Some(1), b: Some(5.5) } // As Hex
00 01 00 00 00 00 00 b0 40
Foo { a: None, b: Some(5.5) }
01 00 00 b0 40
Foo { a: Some(1), b: None }
02 01 00 00 00
Foo { a: None, b: None }
03

Basically the two least significant bits in the first byte correspond to if a or b are None. This gets trickier with struct Bar { a: i32, b: Option<f64> } as the format would use the least significant bit of the first byte to indicate wether or not b is None.
How can I know which elements of the type are Option so I can know the appropriate number of None bits/bytes to reserve.
Thank you.

Hi @chrisvittal, sorry that no one was able to provide help here. If this is still an issue, you could try taking this question to any of the resources shown in https://github.com/serde-rs/serde#getting-help.


Android app freezes; 3 ANR traces provided; issues since CBL 1.3.0

Setup:
Android app
CBL 1.3.0
SQLite storage (not forestDb)
Android OS: 4.4, 5.1, 6.0
Sources of ANR traces and additional info is from Google Play Dev Console
Both replications set to continuous
User presses a button in app, app writes to storage. The updateDocument method is run on the UI-thread. The experience is often slow and sometimes shows an ANR dialog. The issues seem to appear since the last app update with CBL 1.3.0. Before that the updateDocument method took less than 30ms on average.
Reproducibility: Difficult to reproduce. To me it looks like that there is a deadlock in CBL/SQLite code
Traces
Similar issues posted on https://forums.couchbase.com
https://forums.couchbase.com/t/cbl-freezes-app-full-anr-trace-provided/10048/1
https://forums.couchbase.com/t/updating-a-document-takes-375x-longer-after-a-few-updates-app-freezes/9974
https://forums.couchbase.com/t/android-app-freezes-because-of-cbl-because-of-sg-cb-server/9997

@benjaminglatzeder,
CBL Android enables multi SQLite connections, maximum is 4. However write connection is 1. From stack trace, pull replications are middle of deleting or inserting record while main thread is waiting for connection. I recommend to use Database.runAsync() or android's AsyncTask to insert document instead of inserting document from main thread.

Main thread to wait: https://gist.github.com/benjaminglatzeder/51583115746a0d26032b3f6d1a0fafc8#file-gistfile2-txt-L175-L217
Pull replication to delete record in SQLite: https://gist.github.com/benjaminglatzeder/51583115746a0d26032b3f6d1a0fafc8#file-gistfile2-txt-L1037-L1072
Pull replication to insert record in SQLite:
https://gist.github.com/benjaminglatzeder/51583115746a0d26032b3f6d1a0fafc8#file-gistfile3-txt-L1418-L1445

Before I read your comment I build a sample Android app with CBL 1.3.1 and data is only saved locally.
The data modell in the project is PersonPOJO. Persons are stored in a single document. I know that this is bad practice but I'd like to mimic the data modell in my production app. The JSON looks like this after 2 persons were added:

Todo: Test sample app

Update:
@hideki I very much value your time and there is a good chance that you do not have to invest anymore time on this issue. I'm in contact with enterprise support and the problem might be that revisions are never pruned when a document has conflicts. The reason is that I never solve conflicts on the client-side. I will update this thread if implementing conflict solving will have fixed the issue

@benjaminglatzeder,
Thank you for your update. Please keep posting status. Thank you!

There were no issues in the last days. Implementing conflict resolution on the client side fixed the issue. Here is how I implemented it for my use case. Closing this issue.


GPC-614 merge upstream go-gtp changes into 1nce fork

@dzvancuks @oicnysa merge from wmnsk/go-gtp

@gordeyg did you run FT with these changes?

yup, FT + UT for each project

@AlexanderKudzevich fyi


Move translation to where variable is defined. You can't translate va…

You can't use variables inside $t(...), Magento won't place the translations in the js-translation.js file.
See: https://devdocs.magento.com/guides/v2.4/frontend-dev-guide/translations/translate_theory.html
Magento uses a couple of regex to find translations that are used in javascript, see https://magento.stackexchange.com/a/322432. When you use a variable inside a $t(...) it can't find the value of the variable to translate.

Thank you very much for your submission @mediactbv !
Code rows you refer to are ~4 years old , so it's quite old issue, but anyway it's always good to improve it.
But our policy doesn't allow to contribute directly to release branches.
So please feel free to re-create your pull request for develop branch (see more details here : https://github.com/buckaroo-it/Magento2/blob/master/CONTRIBUTING.md) or let us know and will we commit it by ourselves.


minSdkVersion & typo

Since firebase_ml_vision is discontinued, I am migrating to your plugin. Thereby I realized that google_ml_kit currently requires minSdkVersion >= 26, while firebase_ml_vision supported Version 16 onwards. Therefore I am wondering, if minSdkVersion 26 is really necessary or if it can be lowered, to increase compatibility. If this is not possible, I suggest adding a hint in the readme file.
Furthermore during testing I found a typo in the Language Detection Code: identifyLanguange

what minSdkVersion do you recommend?

I think 21 is possible.

Ok, will send a PR

Thanks for your quick response! I tested the example app with minSdkVersion 21 and it seems to work as expected. A lower version was not possible, since the example app depends on the camera plugin, which requires minSdkVersion >= 21. But at least for my purposes supporting Android 5.0 and higher is more than enough.

Let's close this issue after this PR is merged: https://github.com/bharat-biradar/Google-Ml-Kit-plugin/pull/25


Reword "same size" language for lists

Users are frequently confused by the "same size" error; this change rewords the errors to specify that the lists must have the same number of entries. Hopefully it helps reduce confusion.

So I can't just merge this because of how Crowdin integration works (it would automatically delete the altered translations), but I'll go through the process to make this happen.

RIP, I was trying to save you the effort. 😂

Addressed in #861


42 shape validation

Implements the extended shape validation syntax as described in https://github.com/BAMWelDX/weldx/issues/42
example schema implementation in debug/test_shape_validator-1.0.0.yaml
A few notes:

we should rework the behaviour to correctly yield ValidationError as described here https://asdf.readthedocs.io/en/2.6.0/asdf/extensions.html#adding-custom-validators
add more tests
how to handle the case of ( ) before ... ?
I think this should be allowed: [1, (n), ...]
remove : use in code ?

Hello @CagtayFabry! Thanks for opening this PR.

In the file tests/test_asdf.py:

Line 386:5: F841 local variable 'test_read' is assigned to but never used

In the file weldx/asdf/tags/weldx/debug/test_shape_validator.py:

Line 8:1: F401 'weldx.asdf.validators.debug_validator' imported but unused
Line 8:1: F401 'weldx.asdf.validators.wx_unit_validator' imported but unused

Do see the Hitchhiker's guide to code style


added unwanted functions to forbidden api

Every async function in CompletableFuture without Executor -argument is forbidden and must not be used.

Codecov Report

Merging #5892 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #5892      +/-   ##
============================================
+ Coverage     82.91%   82.94%   +0.02%
- Complexity    12131    12132       +1
============================================
  Files          1425     1425
  Lines         46476    46476
  Branches       5728     5728
============================================
+ Hits          38537    38550      +13
+ Misses         5259     5246      -13
  Partials       2680     2680

Impacted Files
Coverage Δ
Complexity Δ

...exceptions/GroupByOnArrayUnsupportedException.java
66.66% <0%> (-33.34%)
1% <0%> (-1%)

...o/crate/analyze/SafeExpressionToStringVisitor.java
83.33% <0%> (-16.67%)
7% <0%> (-1%)

.../java/io/crate/analyze/symbol/ParameterSymbol.java
82.35% <0%> (-5.89%)
6% <0%> (-1%)

.../java/io/crate/blob/TransportDeleteBlobAction.java
75% <0%> (-5%)
4% <0%> (-1%)

...or/transport/distributed/DistributingConsumer.java
75.96% <0%> (-3.85%)
21% <0%> (ø)

.../operation/collect/sources/ShardCollectSource.java
82.82% <0%> (-3.54%)
29% <0%> (-3%)

...er/src/main/java/io/crate/sql/tree/AstVisitor.java
26.77% <0%> (-3.15%)
34% <0%> (-4%)

...rc/main/java/io/crate/blob/BlobTransferTarget.java
81.09% <0%> (-3.05%)
25% <0%> (-1%)

...ava/io/crate/data/AsyncOperationBatchIterator.java
86.76% <0%> (-2.95%)
22% <0%> (ø)

...src/main/java/io/crate/analyze/symbol/Literal.java
84% <0%> (-2%)
50% <0%> (-1%)

... and 22 more

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 4a9c013...5862250. Read the comment docs.


Table wrapper overflow bug

Overview of the problem
Buefy version: [0.3.2]
Vuejs version: [2.3.3]
OS/Browser: Windows/Chrome
Description

When using dropdown component in table component, after dropdown collapse under bottom of the table, we can see scrollbar at the right (overflow issue).
The problem is in .table-wrapper:
.table-wrapper {
    overflow-x: auto;
    margin-bottom: 1.5rem;
    .table {
        margin-bottom: 0;
    }
}

fix:
.table-wrapper {
    overflow-x: visible; //change to visible
    margin-bottom: 1.5rem;
    .table {
        margin-bottom: 0;
    }
}

Steps to reproduce

Use table component
Insert dropdown component inside table
Click on dropdown button to collapse it under table

Expected behavior

Actual behavior

Thanks for the detailed issue!
The overflow-x: auto property is to automatically show scroll for overflowed tables, like this:

That's why I can't keep it always visible, but I'll make it auto only for tablet or less, probably gonna fix.


Use github actions to cut release branch

This currently only doesn't work because it relies on self hosted runners. Our Actions queue has actually been low enough that we can probably get away with just using hosted. It might just slow things down a bit, but that's better than dealing with environmental problems.
I tested this against my fork and it produced the following commit against master (note, this was the only commit produced and I've since force pushed my branch back to apache/beam head): https://github.com/damccorm/beam/commit/0ebe7edec167559adb82b1ded1c03ac253fdeab2
and against the release branch that it created: https://github.com/damccorm/beam/commit/0548e6676fd0ba59aaae9f70bd35ed6280fc3591
For comparison, here are the corresponding commits from the last release against master: https://github.com/apache/beam/commit/eb169f5bb95a8e91112f2d8122f5e7068039713b
and the release branch: https://github.com/apache/beam/commit/3b44dc61bc515bdee1d810402b155780b752226a

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

[ ] Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
[ ] Update CHANGES.md with noteworthy changes.
[ ] If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

R: @jrmccluskey (this should keep us from future problems with this script 🙃 )

Commented on the postmortem doc - could it make sense to have two separate actions with simpler scripts? Advancing the SNAPSHOT version on the master branch is actually separable from cutting the branch. Only advancing the version should require a push to master and isolating that could potentially simplify everything to reduce the likelihood of problems.

I don't see a ton of value in separating it out since these will always be run together (and I don't personally see them as being particularly complicated scripts). In my mind, it just creates an extra button to press. One option would be to give users an option to skip the job that pushes to master via an input (a la https://github.com/apache/beam/blob/d7ded3f07064919c202c81a2c786910e20a834f9/.github/workflows/build_release_candidate.yml#L16)
I don't feel strongly one way or another; if you do, feel free to put up a PR

I mean in the event that "cut release branch" encounters an issue after advancing master branch, there's no reason to rerun the whole thing. Trying to revert and rerun appears to have been a trigger for the deletion.

I agree that extra button is not useful.

This should be a good middle ground - https://github.com/apache/beam/pull/26793 - it defaults to chaining them, but you can opt out


[BUG][Spark] Reliability issues when upserting data to ADLS using Delta in Synapse Analytics

Bug
Which Delta project/connector is this regarding?

[x] Spark
[ ] Standalone
[ ] Flink
[ ] Kernel
[ ] Other (fill in here)

Describe the problem
When upserting data from an Azure Synapse Analytics Spark session to ADLS Delta tables, we are seeing significant reliability issues in our job. In particular, when doing the Delta upsert, we are seeing executors will fail by running OOM. Once enough executors fail this causes the job to fail, bringing down the pipeline.
Our data size is not particularly large.
According to Azure support, our nodes are reaching maximum memory and CPU usage, not spilling to disk, then shutting down.
We are not sure if this is due to an issue with Delta upsert, Synapse analytics runtime, or something else. However, we have been able to isolate the issue to the writing step to ADLS by writing to a Synapse data catalog table, then reading that in to materialize the data and break the lineage.
Steps to reproduce

Create table in Synapse data catalog with around 40 GB of data in ~70 columns. Must contain columns named "year", "month", "file_date" and 2 PK columns "pk_1" and "pk_2" - "pk_1" is not unique, and is clustered on a few values, whereas the combination of "pk_1" and "pk_2" is unique.
Use the below code to overwrite into Delta table:
def overwrite_table(df: DataFrame):
    print(f"INFO: Creating or reinitializing the Structured Delta table")
    df.write\
        .format("delta")\
        .option("overwriteSchema", "true")\
        .partitionBy("year","month","file_date")\
        .mode("overwrite").save(targetLakePath)

Load a similarly sized amount of data with same structure into another data catalog table.
Use the below code to upsert the data into the same Delta table:
   mergeList = []

    mergeList.extend(hive_partition_cols)
    mergeList.extend(pkList)

    mergeColumnsList = [ f"tgt.{mergeColumn} = src.{mergeColumn}" for mergeColumn in mergeList ]
    mergeColumns = " and ".join(mergeColumnsList)

    print(f"INFO: merge condition: {mergeColumns}")

    dtStruct = DeltaTable.forPath(spark, targetLakePath)

    colsdict = determine_cols_to_update(df)
    dtStruct.alias("tgt") \
        .merge(
            df.alias("src"),
            mergeColumns
        ) \
        .whenMatchedUpdate(set = colsdict).whenNotMatchedInsertAll() \
        .execute()

where the function referenced is:
def determine_cols_to_update(df: DataFrame):
    colsdict =  {}
    for dfcolumn in df.columns:
        if dfcolumn not in ['Create_Audit_ID','Create_Audit_Date','Update_Audit_ID'] :
            colsdict[f'tgt.{dfcolumn}'] = f'src.{dfcolumn}'
        elif dfcolumn == 'Update_Audit_ID':
            colsdict[f'tgt.{dfcolumn}'] = f'{auditID}'
    print(f"Updating on the following cols: {colsdict}")
    return colsdict

Observed results

The execution runs fairly quickly, but seems to have executors failing. This can be inferred because tasks fail in groups that are the same size as the # of vCores allocated to each node, and the tasks fail with the message:
ExecutorLostFailure (executor 21 exited caused by one of the running tasks) Reason: Container from a bad node: container_e01_1718824424890_0001_01_000002 on host: vm-0a304008. Exit status: 137. Diagnostics: [2024-06-19 20:52:33.201]Container killed on request. Exit code is 137

I have also just recently seen a failure, which may be a Delta issue:
Job aborted due to stage failure: Task 99 in stage 84.0 failed 10 times, most recent failure: Lost task 99.9 in stage 84.0 (TID 13309) (vm-cb733359 executor 23): org.apache.spark.sql.delta.DeltaFileNotFoundException: File path abfss://{ADLS}.dfs.core.windows.net/structured/{table_path}/_temporary/f2a946f1-f9c3-4e21-a78b-accb192568bf/bitvector-files/year=2024/month=05/file_date=2024-05-01/part-00450-430a3400-ff55-4193-bfbb-74048082d1ae.c000.snappy.parquet.bin

I have not deleted this folder location nor truncated the Delta log.
Expected results

I would expect Spark to spill to memory or disk if required, rather than fully exhausting the resourced of nodes. There is currently no spillage.
I would also not expect Spark to generally need to spill in this case because the amount of data is so small relative to the memory available to the nodes, but Spark does keep multiple copies of data so spillage may be needed.
I would also expect Delta to be able to find the files in the Delta temporary folder, which doesn't seem to be properly happening. This could be happening due to the executor failures, but they don't seem to be happening side-by-side.
Further details

Our incoming dataset is around 40 GB before being loaded into Spark. This data is loaded into ADLS as Parquet without proper partitioning currently, which could effect how it is read in. We read it using last modified timestamps, but cannot apply Hive partitioning to it. Copying the data and applying proper partitions didn't seem to resolve failures.
We are upserting into about 72 GB of data. Our target data is partitioned into 23 partitions, but all are used in the upsert.
We have run into failures with various node sizes and amounts. We are currently using 6 XXL Azure nodes (64 vCores, 400 GB memory each) and still seeing these failures. The node size or amount doesn't seem to significantly effect the failures.
We have tried various optimization techniques including salting and repartitioning. We have not tried z-ordering, but can try that if it might address the root cause.
We have also tried doing a MERGE delete, then append, and we've seen node failure issues with this approach as well.
Environment information

Delta Lake version: 2.4
Spark version: 3.4
Scala version: 2.12.17

Willingness to contribute
The Delta Lake Community encourages bug fix contributions. Would you or another member of your organization be willing to contribute a fix for this bug to the Delta Lake code base?

[ ] Yes. I can contribute a fix for this bug independently.
[x] Yes. I would be willing to contribute a fix for this bug with guidance from the Delta Lake community.
[ ] No. I cannot contribute a bug fix at this time.

It seems executor out of memory issue while running merge command. Possibly AQE isn't applied for the join query.
It could be merge source materialization which is enabled by default in delta 2.4.


[bug] Confirm widget left/right keys handling

Hey @liamg! 👋 Hope everything is going great!
Just noticed that, in the confirm window, pressing left/right keys will just update the buttons' display. But the actual selected component will still be the one picked upon first render.
Reproducing it is straight forward. Just run https://github.com/liamg/flinch/blob/main/_examples/confirm/main.go press the right key (the "No" button will be apparently selected) and press Enter. Confirmed: true will be printed out.
Cheers!

Thanks, this is fixed in v0.0.18


refactor: consider removing dependency from gstd

To avoid mixing different gstd versions.
Sails's abstractions could be implemented directly in gstd under some feature or some sub-crate

the idea was declines in favor of checking multiple gstd dependencies with different versions https://github.com/gear-tech/gear/pull/4159


Exit Code is 0 for failing builds

I have a Quasar project and I am using this extension to generate SSG pages.  The pages are uploaded to the AWS servers (S3 behind a CloudFront CDN) and everything is great!
Today, there was an error in one of my codes (used window without checking if process.env.SERVER is true for SSR).  The error is not showing up during quasar build -m ssr build time.  It shows up during generate phase via quasar ssg generate --force-build.
Apparently there is a bug that even a failed ssg generate will have an Exit Code of 0, which is standard for success.  So, since my build script was relying on the Exit Code (i.e. echo $? in bash) of this generate script (0 being success, anything else being failure), it didn't stop and we found the issue only after it was uploaded to production.
I tested it manually to verify it's the issue -- for a compile/build time error, quasar build -m ssr is returning a non-zero Exit Code properly (e.g. 1 for a compilation error), but during the generate time, the extension doesn't return a non-zero Exit Code when it fails to generate the pages.

Wow, I really appreciate the quick turnaround @freddy38510! 👏 🎉
Your fix works great!
I just tested that on 1.0.4 (that I was on) this issue happened and quasar ssg generate && echo "continuing to push" did print out continuing to push" even when there was a SSR rendering error.
On 1.2.0, with the same SSR error, quasar ssg generate --fail-on-error && echo "continuing to push" didn't print out the continuing to push message as expected.  Thanks. :)
PS. I agree that --fail-on-error should be an optional flag for now and V1.  It would be good to have a plan to change its default value to true for V2, as it would be the natural thing to happen.  Thanks again!


can not read info.plist

how to solve this problem ? the demo not compile by a problem in .plist file

What the compiler says you? I don't see any problems in Info.plist

error: could not read data from
'/Users/Creapix/Documents/Smile-Detector-CameraViewController-master/Demo/SmileCameraViewController/SmileCameraViewControllerTests/SmileCameraViewControllerTests-Info.plist':
The file “SmileCameraViewControllerTests-Info.plist” couldn’t be opened
because there is no such file.
2015-04-09 9:24 GMT-03:00 Maxim Makhun<EMAIL_ADDRESS>
What the compiler says you? I don't see any problems in Info.plist
—
Reply to this email directly or view it on GitHub
https://github.com/MaximAlien/Smile-Detector-CameraViewController/issues/4#issuecomment-91213838
.

Sure? File is located in /Smile-Detector-CameraViewController/Demo/SmileCameraViewController/SmileCameraViewControllerTests/SmileCameraViewControllerDemoTests-Info.plist. Check using Finder or any other file manager.

yes file is located in /Smile-Detector-CameraViewController/Demo/
SmileCameraViewController/SmileCameraViewControllerTests/
SmileCameraViewControllerDemoTests-Info.plist.
2015-04-09 9:39 GMT-03:00 Maxim Makhun<EMAIL_ADDRESS>
Sure? File is located in
/Smile-Detector-CameraViewController/Demo/SmileCameraViewController/SmileCameraViewControllerTests/SmileCameraViewControllerDemoTests-Info.plist.
Check using Finder or any other file manager.
—
Reply to this email directly or view it on GitHub
https://github.com/MaximAlien/Smile-Detector-CameraViewController/issues/4#issuecomment-91217929
.


Add imprint

I am closing this PR, since those changes were already merged in dev from another source.

Does it close #66?

I tried to solve the conflict squashing instead of creating the merge commit but I did it wrong, so now the pull request's code was merged in commit 3735982 to dev, but GitHub did not detect this. Therefore I am closing the pull request.


limit on 'with_' as well as relations in global scope

I apologize for posting here, it seems like there is generally limited knowledge of Orator on the interwebs, and this seems like the best place where there is concentrated knowledge.
I have run into 3 major issues leveraging Orator in our application.  The first two, noted in the title:

It appears that when using the with_ method on a class, that it can only receive 3 relations, either that, or only 1 nested relation.  I am attempting 5 relations, and it quieting ignores #4, and #5.
In an attempt to work around item 1, I tried adding a global scope per the docs like below, which quietly ignores as well:

class MyModel(Model):
    @classmethod
    def _boot(cls):
        cls.add_global_scope(lambda query: query.with_('relation1', 'relation2','relation3'))
        super(MyModel, cls)._boot()

There are a few stackoverflow posts about it, but I am wondering if there is a best practice in deploying Models in a highly relational environment where the same models are relationships to many other Models.  I keep running into the recursive include problem.

Thank you, if there is somewhere else I should post something like this, please advise.

@mrsollis  In order to limit the with_ you must specify in your model that the relationship is limited meaning
Class <Original Model Name>(Model):
    @<relationship decorator>
    def <Relation Name>():
        <Relation Model Name>.limit(<limit amount>)

And in your code:
    <Original Model Name>.with_(<Relation Name>)
This will do the trick.

@mrsollis  I think I misinterpreted your issue, so we should talk more about it so I understand your point

hello @zardilior thank you for the reply.  for the 1st problem described it more of a problem like this:
<Original Model Name>.with_(
     <Relation 1 Name>,
     <Relation 2 Name>,
     <Relation 2 Name.Subrelation 1 name>,
     <Relation 2 Name.Subrelation 2 name>,
     <Relation 2 Name.Subrelation 3 name>)

Hope that makes sense.
Regarding problem #3, is there any documentation around best practices of how to structure a relational data model with ORATOR Models?  for example, in a multi-tenant environment, you would have say a profile model and an tenant model that would appear on all of the other data models, but if included in 1, it can not be included in another because it creates a circular reference and the app dies.  Thanks.

About problem 3 can you give me any examples?


Finish IP authentication code for EDS

There is a breaking change in recent versions of the "search" method of the gem; I will have to investigate that before upgrading.  However, I will need to upgrade before using IP auth in production due to some bugs in old versions of the gem.

In addition, the gem's retrieve method has been changed to request_retrieve in recent versions.

Fix #173 first!

The most recent EDS API gem made this really easy!


Q.JS dependency

Hi
could you double check if q.js dependency was fixed in V-0.3.1?  I am still seeing the 'defer' undefined error once I remove Q.js link from my markup.
I am also seeing unless I configure  "content-type:  application/json" in the header,  SharePoint REST service returns xml payload instead of json, so the code bombs out at the first character of JSON.parse(response) .  I am not sure if this is a particular issue with Microsoft SharePoint service.

Hi,
can you check this commit for the Q dependency if its then working?
For the second point: o.js assuming that your service returning JSON by default or was using the v3 property $format=json. Maybe we can set it as default value for all v4 services. Are you sure it is safe to set this header by default?
Regards
Jan

Hi
No dice.
o.js:455 Uncaught TypeError: Cannot read property 'defer' of null
Also for the second point,  I mentioned "content-type' but it is actually 'Accept' header that I need to set up. If you look at MS site page that explains SharePoint service page here, https://msdn.microsoft.com/en-us/library/office/dn531433.aspx

Accept header : Specifies the format for response data from the server. The default format is  application/atom+xml. Example:

Okay, here another try. Can you pls test this version (sorry, I have no real test service any more)?
I think we could safely set the accept header to JSON. I will implement this enhancement as soon as we fix the promise bug.

Thank you, it is finally working.


Question: New default body FontSize - 14 epx or 15 epx?

Settings and Feedback hub, has moved to using a 14 epx value for it's main body FontSize.  Previously this default font size was 15epx.
Will the guidance and type ramp be updated along with WinUI 3.0?

I think this change to the defaults has already been made as part of the Fluent Standard sizing introduced with Windows 10 version 1809: https://docs.microsoft.com/en-us/windows/uwp/design/style/spacing

I think this change to the defaults has already been made as part of the Fluent Standard sizing introduced with Windows 10 version 1809: https://docs.microsoft.com/en-us/windows/uwp/design/style/spacing

Thank you for that article link - I knew I wasn't imagining the font size change


Proposal: require service_id on last_operation for statelessness

Currently Service Brokers have to choose at max 2 out of these 3 desirable properties:

Offer multiple Services per Service Broker
Statelessness
Asynchronous operations support

Why?

When Service Brokers offer multiple Services, incoming requests need to be passed to the right Service.
To determine which Service should handle a request, the service_id parameter can be used. Currently service_id is required for all operations except for last_operation, where it is only a suggestion. Note that last_operation is only used for asynchronous operations, hence the 2 out of 3 choice.

A workaround exists. A Service Broker can "broadcast" the last_operation call to all its Services, but this complicates the code and slows down the operation.
I propose we make service_id required in the last_operation call to rid us of this problem.

@jtwaleson I'm not 100% sure but I think you're mixing "service" with "service instance" at times so I might not be understandings you.  However, let me make these points:

on last_operation while service_id is optional, the operation string is required if it was returned from the broker. This means that if a broker requires a service_id in order to properly route messages then it can always include it as part of the operation string - thus making it (sort of) required in last_operation messages to that broker.
for consistency I tend to agree with you that it seems like service_id should be required, but we can't turn it into a MUST w/o breaking backwards compatibility - or move to a v3 of the spec. We've had some talks in the past about radically changing how async processing is done if/when we get to a v3 of the spec so I think keeping this issue in mind for that time would be good.

@duglin it is purely about Services, not Service Instances.
Using operation to add the service_id seems like a more robust workaround. I have implemented that instead. See https://github.com/jtwaleson/example-service-broker/commit/298957fe0d2669438e7e9020353de87d2fa5fa28
I understand the concern with backwards compatibility. The spec currently says:

Note: Although the request query parameters service_id and plan_id are not mandatory, the platform SHOULD include them on all last_operation requests it makes to service brokers.

If all existing implementations already adhere to this, perhaps waiting for v3 is not required, but I have no insight into this.

Last-operation request already has this field, but it is optional. k8s Service Catalog and CF both support this in the latest releases.
https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#polling-last-operation
Suggesting we close the issue now, but will consider this feedback for v3.

@avade Makes sense, thanks. As the spec suggests the field and both k8s & CF currently support it, I think service brokers that require it can be deployed with care (that is, proper documentation and CI) until v3.


Publishing compatibility

Two relatively small changes that were needed to integrate with our project. Feel free to close this PR if these changes divert from your strategy, but figured I'd contribute back what was necessary for me.

Add JVM resources as parameter to the kt_db_android_library macro and pass to the underlying kt_jvm_library.

Only apply tags to the final target to ensure that intermediate targets are bundled properly when capturing the transitive closure.

This is a consequence of how the vaticle/bazel-distribution publishing rules capture the transitive closure. If the dependency has a maven_coordinates tag, then it will be added to the POM, otherwise it will be bundled into the artifact to publish. So, when the tags are applied to the intermediate targets, they won't be properly bundled into the artifact.
https://github.com/vaticle/bazel-distribution/blob/a4ff3778d65d6f4c563b70c096d150f7028800ce/maven/rules.bzl#L189-L199

Thank you for this! I will verify compatibility internally and get back.

Hi @sugarmanz after verifying internally unfortunately we could not merge this since we need to pass tags for a different optimization internally. We do hope to remove later but no active plans yet. Thank you for trying and submitting this PR.


Errors with "demos" codes

Hi @MaximilienNaveau  !
I am in internship at LAAS with Olivier Stasse. I have build a Bolt, and now I am trying to apply those codes on the robot. I am currently trying to use the “demos” codes. I forked the repo (https://github.com/PaulRouanet/bolt.git)  in order to create “_test” versions that I am modifying. My main issue is that when I am running “bolt_demo_calibration” or “bolt_demo_sensor_reading”, I always have the same message :
“ERROR: The slider box is not responding correctly, 10 iteration are missing.”
I have seen with Olivier, he think that communications between Masterboard and Micro-Drivers are the reason of this error.
Firstly, we have though about message printing (on sensor_reading), but when we comment the lines, nothing change.
Then, he though about real time priorities problem. That’s why he modified “thread” codes in real_time_tools (fork : https://github.com/PaulRouanet/real_time_tools.git). He give the first priority to the reading (-80).
Do you have an idea on how to solve this error message ?
And then, is there the “bolt_demo_calibration” code that allow us to calibrate our Bolt ? And finally, which code allow us to make walk Bolt (is it bolt_demo_pd ?) ?
Best,
Paul

I have discovered that a "slider box" may be a hardware box of slider. And of course I have not got it, so it can't responding correctly ...
Do you know the role of this box ? Is it used to move some actuators ? And how can I have one if I need it ?

Dear Paul Rouanet,
I am sorry I did not see your message earlier.
The slider box is this box an arduino plugged to 4 linear potentiometers and a e-stop button. The connection to the computer is done via serial port (usb):
https://github.com/open-dynamic-robot-initiative/bolt/blob/master/src/bolt.cpp#L80
The serial_reader_ object is responsible for the communication. It allows us to have a soft e-stop during the experiment.
You can simply remove the object from the code if you do not have the box itself.
Sorry this part of the hardware do not seem to be documented yet... I will ask if this can be fixed.
Otherwise you should not have to increase the priority of you executable at all, the code does it for you.
You might just have to run it with sudo though in order to properly connect to the master-board.
Let me know if you succeed.

Hi Paul Rouanet,
Here is some documentation about the wiring of the e-stop. It's important to use a pull-up line via a resistor.

Note that these days we use the Arduino Leonardo.
Let me know if you have any further questions.
Best,
Julian

And then, is there the “bolt_demo_calibration” code that allow us to calibrate our Bolt ? And finally, which code allow us to make walk Bolt (is it bolt_demo_pd ?) ?

For these question in order:

The demos in general are simple controller that demonstrate the capabilities of the API not the robot itself.
The bolt_demo_calibration simply shows you what happens when you calibrate the robot.
In order to make Bolt walk you need a more advance controller which is not the scope of this package. You need:

An estimation software providing you with the centroidal dynamics and other proprioceptive informations.
A reactive planner that will plan for centroidal dyanmics and foot step generation.
A whole body controller in order to track the above trajectory.
etc.
We do have a solution but it is ongoing work and not very well documented/packaged.

Ok thanks to both of you !
I removed everything related to the slider box in bolt.cpp and I can do some tests. LAAS doesn't plan to make this box for the moment.
And for the last answer, ok, I am going to check that

Maybe this page will become handy for you as well: https://github.com/machines-in-motion/machines-in-motion.github.io/wiki/bolt_hardware

Dear Paul Rouanet,
I am sorry I did not see your message earlier.
The slider box is this box an arduino plugged to 4 linear potentiometers and a e-stop button. The connection to the computer is done via serial port (usb):
https://github.com/open-dynamic-robot-initiative/bolt/blob/master/src/bolt.cpp#L80
The serial_reader_ object is responsible for the communication. It allows us to have a soft e-stop during the experiment.
You can simply remove the object from the code if you do not have the box itself.
Sorry this part of the hardware do not seem to be documented yet... I will ask if this can be fixed.
Otherwise you should not have to increase the priority of you executable at all, the code does it for you.
You might just have to run it with sudo though in order to properly connect to the master-board.
Let me know if you succeed.

Dear @MaximilienNaveau and @jviereck thanks for the quick answer.
As for the thread, checking at the code of real_time_tools, I see the priority set for the RT_PREEMPT patch for linux and for xenomai for nothing for the usual Linux Kernel. But even in this later case, it is possible to change the scheduler and affect real-time priority. They are less real-time guarantees (like priority inheritance and priority handling of interruption) but it is still better.


Explicitly request the release of container's resources and trigger post-stop hooks

Extend the Container API with "Delete", and make use of it on interactive "podman run" sessions, so when they terminate the container's resources are released and the post-stop hooks run.
Not doing this may cause the OCI runtime to keep holding some resources until the container is removed. This is very visible with Kata, as the QEMU process and its proxy will keep running until "delete" is called for the runtime.

Hm. Maybe we should make delete() part of Cleanup(), which would not
require any changes to run and would also handle detached containers.
I like the delete API, though. Full review coming in a few hours.
On Tue, Sep 18, 2018, 05:19 Atomic Bot<EMAIL_ADDRESS>wrote:

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/containers/libpod/pull/1498#issuecomment-422320589,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHYHCAe5aKB2uafLdife7DiH2u4Z0DxTks5ucLq3gaJpZM4WtgO4
.

Yes lets do this in the podman container cleanup code.

Hm. Maybe we should make delete() part of Cleanup(), which would not require any changes to run and would also handle detached containers. I like the delete API, though. Full review coming in a few hours.

I considered this option too, but reading the code I got the feeling that Cleanup() was intended just to clean up network and mountpoints. I'm fine with both approaches though.

@slp We want to extend cleanup to make sure that the container is fully cleaned up.  We also want to extend it to record the exit code for the container in to the libpod database.
podman container cleanup is called when every container exits, so we don't have to worry about handling containers in run as well as start and containers that are run in daemon mode.

bot, add author to whitelist

@slp BTW I am glad someone is playing around with podman and Kata Containers...

I think you should add the Delete method and then have cleanup cal it.

@slp BTW I am glad someone is playing around with podman and Kata Containers...

The combination of podman + Kata is amazing. It'd be even better if the latter supported labeling the shim, proxy and QEMU processes, as an additional security layer, but I guess they'll eventually get there.

@slp I'm going to poke at this over the weekend - I think I know the proper way forward, but it might take a bit of work to implement.

@slp Opened #1531 which should address the problems with restart

@mheon Ack, closing this one (next time I'll open an issue instead of a PR).

@slp Thanks for the contribution anyways - this issue ended up growing a lot of unexpected complexity under the hood.

@slp We like PR's and Issues.  Either way thanks for contributing.


JS: be less conservative about incomplete nodes in prefix sanitizers

For the request forgery query, we treat any incomplete node as a prefix sanitizer, on the grounds that it might contain # or ?.
We often got false negatives from code like:
let base = require('./config').base;
request(`http://example.com/${base}/${tainted}`);

Technically, base could end with # or ?, but it seems unlikely given the slash separator between ${base} and ${tainted}. This PR realizes that idea by specifically not sanitizing taint in the above example, simply because it is prefixed by /.
This query console run shows some prefix sanitizers that are no longer in effect. Evaluation on typescript-full shows some new results. I can run a full eval on default.slugs if anyone wants.
Fixes https://jira.semmle.com/browse/ODASA-7494

I agree with the core idea, but I was actually considering restricting the query instead.  I find that we get a lot of FPs from URLs where the tainted path component really isn't that bad:
request(`http://example.com/${base}/api/v2/${tainted}`)

Perhaps we should consider splitting the query into two: one for the host, and one for the path. The former is obviously the most severe. Perhaps there is a middle ground where the first path component is tainted since that gives you access to all endpoints on the remote server.

Perhaps there is a middle ground where the first path component is tainted since that gives you access to all endpoints on the remote server.

You can use ../ and end with ? to gain full control of the path.

You can use ../ and end with ? to gain full control of the path.

Yes, that will probably work in many cases. No middle ground then.
I will review as is, but I would like to see the alert changes for a default.slugs evaluation before merging this.

Perhaps we should consider splitting the query into two: one for the host, and one for the path

I agree something else could be done, although I'd go in a different direction. We currently have:

ClientSideUrlRedirect
ServerSideUrlRedirect
RequestForgery

I think these could be recast as:

UrlHostnameInjection (possibly including scheme injection)
UrlPathInjection

I find that the above would be more convenient for customizable queries. I've had to use ClientSideUrlRedirect for things that aren't at all "redirects" but I needed its reasoning about URLs. And fewer queries usually means better performance, which is important.
Anyway, this PRs isn't the best forum for discussing architecture at this level. I'll add it to the agenda for our Monday meeting so we can discuss.

Running evaluation on default.slugs.

Evaluation shows no new results and performance looks fine; those above 5% seem to be noise due to only running a single query. The result from the previous evaluation only manifest with the extended call graph from full TypeScript extraction.


Labels not visible on dark mode

Using a LineChart, the x and y-axis labels aren't legible on dark mode since they are in black color. The axis config colors aren't applied to them.
LineChart(
            lineData = minutesReadData,
            color = MaterialTheme.colors.primaryVariant,
            modifier = modifier
                .height(250.dp)
                .fillMaxWidth()
                .padding(vertical = 32.dp, horizontal = 32.dp),
            axisConfig = AxisConfig(
                showAxis = true,
                isAxisDashed = false,
                showUnitLabels = true,
                showXLabels = true,
                xAxisColor = Color.White,
                yAxisColor = Color.White,
            ),
        )

On Light Mode:

On Dark Mode:

Hey this is something I have planned for future releases but right now I am focusing more on making this library stable.. but will keep this in mind!

Late addition:
As a workaround, I forked the lib and obtained the onSurface color in the LineChart composable:
val labelColor = MaterialTheme.colors.onSurface
yAxis(... labelColor = labelColor)

Then, added a color param to DrawScope.yAxis and DrawScope.drawXLabel which is then consumed during paint object init:
fun draw...(..., labelColor: Color = Color.Black) {
    // ...
    Paint().apply {
        // ...
        color = labelColor.toArgb()
    }
}

Now,

Hey, I have these fixed, Will be putting a new release soon!

This will be fixed in the next release


Multi-fetch adapter

Thanks for this project (and the corresponding AngularJS one).
My express backend is used by an Angular JS web client and an ionic 2+ app.  It seemed to me that meant I needed to use the multi-fetch adapter which you provide for Angular JS and roll my own for Angular.  (If you can see a better option you are welcome to let me know)
I have got it working for my purposes at the moment in this branch and can submit a pull request if you would welcome it.  I haven't submitted one straight away partly because I haven't yet deployed and partly because there are some bits that look a bit hacky (such as this)

Closing as I managed to conflate it with another PR as I still don't understand Git very well.


Incorrect API syntax for Update schedule state

Issue in ./help/segmentation/api/schedules.md
Sample API syntax for Update schedule state reads as curl -X DELETE https://platform.adobe.io/data/core/ups/config/schedules/4e538382-dbd8-449e-988a-4ac639ebe72b and this should be
curl -X PATCH https://platform.adobe.io/data/core/ups/config/schedules/4e538382-dbd8-449e-988a-4ac639ebe72b \
Please correct.

Thank you for submitting this issue. We will review the documentation and make the necessary updates as quickly as possible. Tracking internally in PLAT-124067.

The documentation was reviewed and updated to reflect the correct syntax. Closing this issue.


Add support for negative timecodes

The current implementation of the timecode class does not support negative timecode-values. Theoretically the class should never be used with negative timelines, but assumptions usually lead to unwanted bugs in the future.
Ex of a negative timecode: "-10:00:00:00".

I can take a look at this. Please assign me to the issue.

Hi @leijon,
You have been assigned to the issue.


Remove console messages

The console.log messages are appearing in the console tab.

Please assign me this issue so that I can remove the console.log messages under GSSOC-23

Please wait for today, the mentors haven't been assigned till now

Please wait for today, the mentors haven't been assigned till now

Sure

Please assign this issue to me ,I would like to resolve the issue.

Can you please add the level?


Feature Request: Retrieve entire filtered data set.

Currently you can only retrieve a page at a time, I'd like to be able to apply filtering and retrieve the entire collection.

this.data = allRows.slice(query.offset, query.offset + query.limit)

@kenberkeley I'm not following. Where is allRows coming from? datatable.data contains only the current rows for the page, is there another attribute that holds all the rows with the filter applied?

allRows is just a cache variable for 500 rows

There is no difficulty to implement your scenario

<template>
  <datatable v-if="allRows.length" v-bind="$data" :total="allRows.length" />
</template>
<script>
export default {
  data: () => ({
    columns: [
      // ...
    ],
    data: [],
    allRows: [],
    query: {}
  }),
  created () {
    yourApi().then(allRows => {
      this.allRows = allRows
    })
  },
  watch: {
    query: {
      handler (query) {
        this.data = allRows.slice(query.offset, query.offset + query.limit)
      },
      deep: true
    }
  }
}
</script>

If there are no more question, you might have to close this issue

@kenberkeley Thanks for the input but I still don't think the answers the question I had. vue2-datatable seems to take the given original data set, apply  the filter, and then only retain the records needed for the current page. Your suggestion of doing a slice only gets you a sequential set of what was in the original collection, but when applying a filter, there is no guarantee the results will line up like that.
I'm not sure if I'm not explaining this correctly, but I can't think of another way to describe the scenario than what I did before. If I filter a data set, I'd like to be able to retrieve the new filtered collection without having to iterate through pages. If this still doesn't make sense or you don't think it's a reasonable/feasible ticket I'll just close it. I'm happy to set something up in codesandbox as an example.

It's late in China, I got to sleep. Will give u a satisfying reply tomorrow.

Closing the ticket. I figured out the solution.


Latest version? Changelog?

Homepage says latest version is 2.3.3, docs are on 2.2.0, npm says 2.3.5...

@kbrandwijk
About homepage, yes we are checking it
Docs, it becomes difficult to update on every patch release. But yes, we are looking into this. Its on our list. Very soon this will be resolved

ChangeLog and Release Notes are up-to-date


Add a command to purge old logs

Some Prestashop log tables canb grow too much, this command could purge.

Good idea.
I'm in need for this feature.
With option to act on levels under a specified argument.
fop:clear-log --under-level 3 : remove all logs of levels 1,2 and 3.
Wording sounds not very good...
Also, as option, we can have an option like '--keep-last-24h' to avoid touching logs of the last 24h.
The command may not run without an option.
 --all to remove all levels, all time (except last 24h if specified)
If it's possible, it could be implemented in a way that the cleaning process can be started from a cron or a controller.
new FopCleanLogCommand([1,2,3], ...)


doubt for c++ api

following code,where your definition of those two parameter(data,label) in lsoftmax-inl.h,how python know those params
fc4 = mx.sym.LSoftmax(data=embedding, label=label, num_hidden=10,                                   beta=args.beta, margin=args.margin, scale=args.scale,                                   beta_min=args.beta_min, verbose=True)

It's handled by mxnet python package

can be more detailed? while file?thanks!

It's complex to explain the parameter parsing and how mxnet treat the operator parameters and input symbols. data and label is the input symbol for LSoftmax operator, as well as weight which is not specific written in the code and mxnet also will handle this automatically. Please read the python and c++ code to understand the underline mechanism.


Error: Could not retrieve configure-monitoring.finished event with KeptnContext 0409db55-f409-4316-9d42-3c9935e68cff. Please check your monitoring solution if it has been configured correctly.

Hi Im installing Keptn integration with Dynatrace and Im getting the following error. Any idea why is this happening?
$ helm upgrade --install dynatrace-service -n keptn https://github.com/keptn-contrib/dynatrace-service/releases/download/0.17.1/dynatrace-service-0.17.1.tgz --set dynatraceService.config.keptnApiUrl=$KEPTN_ENDPOINT --set dynatraceService.config.keptnBridgeUrl=$KEPTN_BRIDGE_URL --set dynatraceService.config.generateTaggingRules=true --set dynatraceService.config.generateProblemNotifications=true --set dynatraceService.config.generateManagementZones=true --set dynatraceService.config.generateDashboards=true --set dynatraceService.config.generateMetricEvents=true
Release "dynatrace-service" has been upgraded. Happy Helming!
NAME: dynatrace-service
LAST DEPLOYED: Fri Mar 11 14:11:37 2022
NAMESPACE: keptn
STATUS: deployed
REVISION: 5
TEST SUITE: None
vasudevann1930@cloudshell:~ (psyched-garage-343708)$ keptn configure monitoring dynatrace
ID of Keptn context: 0409db55-f409-4316-9d42-3c9935e68cff
could not find matching event after 10 x 3s
Could not retrieve configure-monitoring.finished event with KeptnContext 0409db55-f409-4316-9d42-3c9935e68cff. Please check your monitoring solution if it has been configured correctly.
vasudevann1930@cloudshell:~ (psyched-garage-343708)$

My Dynatrace tenant was wrong. once I corrected it was ok!


fix podspec file

fix cocoapods error "fatal: unable to find remote helper for 'git+https'"  => add explicit git repo source URL in podspec file

lgtm


Rollback support-1399

Just commented the broken code since its going to be addressed soon

LGTM. I would add some functions to the ones @ivanmalagon suggested to comment:
fetchLayerColumns and the listeners to columnsUpdated changes.

So im going to full revert the PR --> https://github.com/CartoDB/cartodb/pull/13948


Allow sending a result back without navigating up

ResultBackNavigator currently always navigates back, but it would be nice if there's a way to send a result to the previous destination without also navigating back.
My use case is that I have a screen where something can be edited that needs to be communicated back to the previous destination, but the navigation itself only needs to happen when the user presses the back button or the back arrow in the toolbar. The result itself is optional in that sense. In the current implementation I can also workaround this by using my own BackHandler but it would be nice if there would be a little bit more flexibility for returning results.

Hi @hvisser ! 👋
Thank you for reporting.
Yeah I debated with this when developing the feature. I wanted to make it clear that this was a final thing, that it wouldn't work as some kind of stream to pass data through.
At the same time, this is an easy change and I can see some use cases where that might be helpful.
The way would be to split the current method into setResult and navigateBack with no arguments.
Maybe also keep the current method. So it would contain three methods in total.
How does that sound? Do you see a way to improve it even more?
Thanks again for the feedback!

I think splitting the calls would give it that little bit of extra flexibility yes, but I can also understand if you want to keep it strictly for the "this destination could return a single result" case. What I'm doing here might not be super common and it's easy to fall back to backstack entry or BackHandler as well.

Wait but in the end, you also only want to return a single result right? Or am I misunderstanding it?
Hopefully, the name setResult also implies that if you call it multiple times, the last one will be the one actually sent back  since it is a "setter" 😅

No you're right I have a single, but optional, result that I'd like to set one or multiple times, so that would work.,

Then I think this is fine, I will do this change for next release. Thank you again @hvisser

Closing as it was fixed in 1.3.1-beta release!


Lower rubucop version restriction

This makes rubocop updates possible without the need to touch the gemspec (so we can use the new rubocop v0.35). In case there is a rubocop v1.* sometime (with breaking changes), it still prevents this upgrade. Or is there any greater intention behind locking it at ~> 0.34.0?

pronto-rubocop doesn't use an official API, because there isn't one. As such, I can't apply semantic versioning rules. There've been breaking changes in the past :smile: