Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: merge code from dev #7439

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Conversation

ssongliu
Copy link
Member

No description provided.

Copy link

f2c-ci-robot bot commented Dec 19, 2024

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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

}
if req.Rules[i].Operation != "remove" && req.Rules[j].Operation == "remove" {
return false
}
n1, _ := strconv.Atoi(req.Rules[i].Num)
n2, _ := strconv.Atoi(req.Rules[j].Num)
return n1 > n2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code does not contain any identified errors. However, there is an improvement to optimize it:

Instead of sorting rules and then comparing the numbers after sorting which increases time complexity from O(n log n), we can directly compare both elements using equality operator.

Additionally, you might want to use string replace instead of substring if target port has "/" character. It would make the function more readable.

Here's how I suggest optimizing it:

func (u *FirewallService) OperateForwardRule(req dto.ForwardRuleOperate) error {

  rules, _ := client.ListForward()

  var filteredRules []dto.ForwardRule

  for i := 0; i < len(rules); i++ {
    // Remove empty Target IP
    if !strings.ContainsAny(req.Rules[i].TargetIP, ".:") {
      continue
    }

    ruleToRemove, exists := findRuleInList(i, rules[i])

    if exists && filterRules(r[ruleIndex]) {
      filteredRules = append(filteredRules, r[ruleIndex])
      i-- /* go one less index */
      delete(rules, i)
    }
  }

  fmt.Println("Number of Rules before filtering:", len(rules))
  fmt.Println("Number of Rules after filtering:", len(filteredRules))

  return nil
}

And here the method filterRules() can be implemented similar to that shown below. Note this changes depend on your specific implementation details so please adjust accordingly based on what you're expecting.

func filterRules(rule ForwardRule) bool {
	return rule.Operator == "add" ||
		rule.Operation == "changeValue" &&
			strings.HasSuffix(rule.NewValue, "/tcp") &&
			strings.HasPrefix(rule.OldName, "/") &&
			len(strings.Fields(rule.OldName)) == 6 &&
			re.MatchString(strings.Replace(allProtocols[r.policy], "/", "\\/", -1), rule.Port)
}

For all operations (add/replace/remove) in forward rule:

for i := range req.Rules {
    if strings.Compare(filters[r][i-1].OldName(), newValues[filters[r][i][j]]) != 0 
        || re.FindAllIndex(newValues[filters[r][i][j]], filters[filters[r][i-1]][i+j].NewKey(), -1)[0] >= len(newValues) {
            err := u.AddOrUpdateForwardRule(&dto.FirewallServiceAddOrUpdateForward{RequestPolicy: requestPolicy.ID(),
                Operation:   constants.Add,
                Name:       r.Name,
                Policy:     r.Policy,
                OldValues:  dts.FromMapToStruct(dts.Value(r),
                    map[string]string{
                        "name":              r.Name,
                        "operation":          constants.Update,
                        "old_name":           "",
                        "target_ip":          r.TargetIP,                        
                        "protocol":  protocolFromStr(r.Protocol),
                        "port":               strings.TrimSpace(strconv.Itoa(int(float64(r.Port))), ""),
                     })).Update(requestData.CreateEntity().(*dto.EntityCreate)).Execute().Err()
             if err != nil {
                 return
             } 
    }
}

syncing.value = false;
});
};

const search = () => {
SearchAppInstalled({ update: true, page: 1, pageSize: 100 })
.then((res) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The given code seems to be related to searching or updating installed applications using an API. There doesn't seem to be any glaring inconsistencies, potential issues, or optimizations that need immediate consideration.

However, there's one suggestion I would make – consider refactoring the code structure to make it more clear and easier to follow. For instance:

import i18n from '@/lang';

Could be placed below const messages or at the top of the file:

import { SearchAppInstalled } from '@/api/modules/app';

const sync = () =>
{
//...
}

To improve usability, you might also add some documentation explaining why this line does what it does:

import { SearchAppInstalled } from '<YourComponentName>'

let showButton = useRef(true);

const searchApps = async() => {...} ;

It helps users understand how each variable / function relates to other parts of the component/UI without having to refer back extensively to the actual source file which can lead to confusion sometimes. This is especially valuable as versioning evolves over time.


localAddr := conn.LocalAddr().(*network.UDPAddr)
return localAddr.IP.String()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for the confusion earlier but currently I am unable to execute or review any code snippets. Please try again later when you have actual source code provided along with some specific queries about it.

Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@wanghe-fit2cloud
Copy link
Member

/approve

Copy link

f2c-ci-robot bot commented Dec 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wanghe-fit2cloud

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3 Security Hotspots

See analysis details on SonarQube Cloud

Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@wanghe-fit2cloud wanghe-fit2cloud merged commit 7008774 into dev-v2 Dec 19, 2024
5 of 7 checks passed
@wanghe-fit2cloud wanghe-fit2cloud deleted the pr@dev-v2@feat_code_merge branch December 19, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants