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

New file utils.js, moved teardown function #1080

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

AwesomeSauce42
Copy link

@AwesomeSauce42 AwesomeSauce42 commented Oct 13, 2023

Description

Added a utils file and moved the euiBuildTimeAliasTearDown function to it.

Issues Resolved

Works on issue #1061

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

@BSFishy BSFishy left a comment

Choose a reason for hiding this comment

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

Just a few quick things :)

scripts/compile-oui.js Outdated Show resolved Hide resolved
scripts/compile-clean.js Outdated Show resolved Hide resolved
scripts/utils.js Show resolved Hide resolved
AwesomeSauce42 and others added 3 commits October 16, 2023 15:03
Co-authored-by: Matt Provost <mattprovost6@gmail.com>
Signed-off-by: Shashwat Prakash <88332189+AwesomeSauce42@users.noreply.github.com>
Co-authored-by: Matt Provost <mattprovost6@gmail.com>
Signed-off-by: Shashwat Prakash <88332189+AwesomeSauce42@users.noreply.github.com>
Signed-off-by: Shashwat Prakash <88332189+AwesomeSauce42@users.noreply.github.com>
@AwesomeSauce42
Copy link
Author

Just a few quick things :)

Made the changes!

@joshuarrrr
Copy link
Member

@AwesomeSauce42 One of the new commits is missing the signoff. You can fix by following the instructions in https://github.com/opensearch-project/oui/pull/1080/checks?check_run_id=17733550749

Also, please add an entry to the Changlelog file, under the "Infrastructure" section.

Copy link
Contributor

@BSFishy BSFishy left a comment

Choose a reason for hiding this comment

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

Good progress, just a few more small things. Also, like @joshuarrrr said, this should get a changelog entry :)

if (changed) fs.writeFileSync(file, content, 'utf8');
});
}
/* End of Aliases */
Copy link
Contributor

Choose a reason for hiding this comment

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

This marker should remain in the codebase

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should also get an empty line at the end of the file.

Copy link
Author

Choose a reason for hiding this comment

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

alright will change this

@AwesomeSauce42
Copy link
Author

@AwesomeSauce42 One of the new commits is missing the signoff. You can fix by following the instructions in https://github.com/opensearch-project/oui/pull/1080/checks?check_run_id=17733550749

Also, please add an entry to the Changlelog file, under the "Infrastructure" section.

Im not sure what to add under Infrastructure here, would a simple :

  • Add functionality for utils (#1080)
    Be enough? Any help would be great.

Signed-off-by: Awesomesauce42 <shashwatpkm@gmail.com>
@joshuarrrr
Copy link
Member

@AwesomeSauce42 Which issue is this for? The issue number in the description doesn't appear to be valid.

@joshuarrrr
Copy link
Member

@AwesomeSauce42 One of the new commits is missing the signoff. You can fix by following the instructions in https://github.com/opensearch-project/oui/pull/1080/checks?check_run_id=17733550749
Also, please add an entry to the Changlelog file, under the "Infrastructure" section.

Im not sure what to add under Infrastructure here, would a simple :

* Add functionality for utils ([#1080](https://github.com/opensearch-project/oui/pull/1080))
  Be enough? Any help would be great.

In the Changelog entries, we're aiming to capture the impact of the change, so generally more of the "why" than the "how" or "what". So potentially something like "Improve organization of build scripts by isolating utilities (#1080)". (It can go under the Refactor section)

@joshuarrrr
Copy link
Member

@AwesomeSauce42 Just checking in on the progress of this - have you had a chance to look at the feedback, or do you have other questions about the next steps? I'm marking it as a draft for now.

@joshuarrrr joshuarrrr marked this pull request as draft November 7, 2023 22:15
@AwesomeSauce42
Copy link
Author

@AwesomeSauce42 Just checking in on the progress of this - have you had a chance to look at the feedback, or do you have other questions about the next steps? I'm marking it as a draft for now.

Yes, I've made the changes on my local, but I'm having issues with my github pushes, I have been getting the error:
! [rejected] feat/shash -> feat/shash (non-fast-forward)
error: failed to push some refs to 'https://github.com/AwesomeSauce42/oui.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details."

Have been trying to fix this but getting stuck here.

AwesomeSauce42 and others added 2 commits November 8, 2023 15:18
Signed-off-by: Awesomesauce42 <shashwatpkm@gmail.com>
Signed-off-by: Awesomesauce42 <shashwatpkm@gmail.com>
Signed-off-by: Awesomesauce42 <shashwatpkm@gmail.com>
@AwesomeSauce42
Copy link
Author

AwesomeSauce42 commented Nov 8, 2023

Hi, I've made the changes that were required. Could you check this once? I'm still new to this so it took a little time, apologies for the same.

Comment on lines +4 to +7
<<<<<<< HEAD

=======
>>>>>>> cc445de2f11c9b8239d710133673ddc534d73b8f
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there's a merge conflict remnant here

Copy link
Author

@AwesomeSauce42 AwesomeSauce42 left a comment

Choose a reason for hiding this comment

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

Reviewed the changes for merge conflict remnant

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.

None yet

3 participants